-
Notifications
You must be signed in to change notification settings - Fork 37k
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
BIP 350: Implement Bech32m and use it for v1+ segwit addresses #20861
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
8ba1f7a
to
05f8733
Compare
f08b0ff
to
863a5c2
Compare
By implementing Bech32m, I found out that it is not obvious how to locate errors, because the This is not the right place to comment for it, but did not found elsewhere. |
@NicolasDorier That isn't solved by sipa only allowing one polimod based on the witness version? |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
Concept ACK, Approach ACK. Skimmed code but will hopefully go through more thoroughly at a later date. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly tested ACK 863a5c2
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 Agree, a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 936322d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-slightly-tested-ACK 2827cf8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review ACK 2827cf8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 2827cf8
utACK 2827cf8 |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-ACK 3fee858
re-ACK 3fee858 |
…ors.py 5c0210e bugfix: fix bech32_encode calls in gen_key_io_test_vectors.py (Pieter Wuille) Pull request description: This fixes the the calls to bech32_encode in the gen_key_io_test_vectors.py script. Bug introduced in #20861. ACKs for top commit: fanquake: ACK 5c0210e Tree-SHA512: 8e8aee08741619c1700371ca1a8ca05ffdb2f48544d9fd3982f2665f6afb926b126478cf644f15a699f8c7e7da53c2777a56ce7989f05e4a3ef9fbe085f74d5a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some minor nits
const std::vector<uint8_t>& data = r2.second; | ||
assert(hrp == "bc"); | ||
assert(data == input); | ||
if (input.size() + 3 + 6 <= 90) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
'❌_VER15_PROG41': 'bcrt10qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqzc7xyq', | ||
'❌_VER16_PROB01': 'bcrt1sqqpl9r5c', | ||
'❌_VER15_PROG41': 'bcrt1sqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqajlxj8', | ||
'❌_VER16_PROB01': 'bcrt1sqq5r4036', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: obviously unrelated to your changes, but the B
should be a G
. (blame me)
@@ -19,7 +19,7 @@ static void Bech32Encode(benchmark::Bench& bench) | |||
tmp.reserve(1 + 32 * 8 / 5); | |||
ConvertBits<8, 5, true>([&](unsigned char c) { tmp.push_back(c); }, v.begin(), v.end()); | |||
bench.batch(v.size()).unit("byte").run([&] { | |||
bech32::Encode("bc", tmp); | |||
bech32::Encode(bech32::Encoding::BECH32, "bc", tmp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Could add a benchmark for bech32m?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, could do. The performance should be exactly identical to Bech32 though.
Reduced version of the test from master/bitcoin#20861 by John Newbery. Github-Pull: bitcoin#20861 Rebased-From: fe5e495
Reduced version of the test from master/bitcoin#20861 by John Newbery. Github-Pull: bitcoin#20861 Rebased-From: fe5e495
Reduced version of the test from master/bitcoin#20861 by John Newbery. Github-Pull: bitcoin#20861 Rebased-From: fe5e495
This also includes updates to the Python test framework implementation, test vectors, and release notes. Github-Pull: bitcoin#20861 Rebased-From: fe5e495
Github-Pull: bitcoin#20861 Rebased-From: 0334602
Reduced version of the test from master/bitcoin#20861 by John Newbery. Github-Pull: bitcoin#20861 Rebased-From: fe5e495
Github-Pull: bitcoin#20861 Rebased-From: da2bb69
Github-Pull: bitcoin#20861 Rebased-From: 25b1c6e
This also includes updates to the Python test framework implementation, test vectors, and release notes. Github-Pull: bitcoin#20861 Rebased-From: fe5e495
Github-Pull: bitcoin#20861 Rebased-From: 2e7c80f
Github-Pull: bitcoin#20861 Rebased-From: 0334602
Reduced version of the test from master/bitcoin#20861 by John Newbery. Github-Pull: bitcoin#20861 Rebased-From: fe5e495
…dresses (0.21 backport) f2195d7 Backport invalid address tests (Pieter Wuille) 1e96711 naming nits (Fabian Jahr) 7dfe406 Add signet support to gen_key_io_test_vectors.py (Pieter Wuille) 593e206 Use Bech32m encoding for v1+ segwit addresses (Pieter Wuille) 8944aaa Add Bech32m test vectors (Pieter Wuille) 1485533 Implement Bech32m encoding/decoding (Pieter Wuille) Pull request description: Backport of #20861. Also includes #21471. ACKs for top commit: jnewbery: utACK f2195d7 MarcoFalke: cherry-pick re-ACK f2195d7 , only change is version number in doc/bips and new test commit 🍝 fanquake: ACK f2195d7 - performed the backport, changes look sane. Have not tested extensively. Tree-SHA512: 7dc043e44d7cda07d73331a7b49666b9db98c99f2635dab0cfeb45422dbfbe75a7b44d0aff85ef6369d412d8a5041ed0826c86ffdfc13c5fbff74adfe4d91c1a
…dresses (0.20 backport) c0f85fd Backport invalid address tests (Pieter Wuille) c670986 naming nits (Fabian Jahr) 1a4e88e Use Bech32m encoding for v1+ segwit addresses (Pieter Wuille) cf18ac9 Add Bech32m test vectors (Pieter Wuille) 5f9537b Implement Bech32m encoding/decoding (Pieter Wuille) Pull request description: Backport of #20861 for 0.20. Also includes #21471. ACKs for top commit: jnewbery: utACK c0f85fd MarcoFalke: range-diff-only ACK c0f85fd 🐔 Tree-SHA512: e87e52995cb9b555109bc500dba1e844993d881821d2001443b3de9e3ef9050ddb73deefa0c1af732418fa7355a86b875789887c9611c340713b3ad26809d58e
This also includes updates to the Python test framework implementation, test vectors, and release notes. bitcoin/bitcoin#20861 (3/5)
This also includes updates to the Python test framework implementation, test vectors, and release notes. bitcoin/bitcoin#20861 (3/5)
c72c949 blech32: copy ubsan suppression for bech32 to blech32 (Andrew Poelstra) d13fb49 blech32: add test vectors for blech32 and blech32m (Andrew Poelstra) 15a826e blech32: add functional tests for blech32m (Andrew Poelstra) c01e09e blech32: add blech32m format and use it to decode witness v1+ addresses (Andrew Poelstra) 18fcec8 naming nits (Fabian Jahr) 8515f40 Add signet support to gen_key_io_test_vectors.py (Pieter Wuille) b3df66f Use Bech32m encoding for v1+ segwit addresses (Pieter Wuille) 42f43a1 Add Bech32m test vectors (Pieter Wuille) b1d1d94 Implement Bech32m encoding/decoding (Pieter Wuille) c607835 Better error messages for invalid addresses (Bezdrighin) Pull request description: Includes backports of bitcoin/bitcoin#20832 (1 commit) and bitcoin/bitcoin#20861 (5 commits) ACKs for top commit: gwillen: utACK c72c949. Tree-SHA512: af96a6ef31b1cab72b0350197dcb34761e9ffb2ec43685084408b5fafcda0adee1945045003194600372652f7cca65d721bda3ed9b6be7e9543d3199e2cbe145
This implements BIP 350: