Simplify Base32 and Base64 conversions by sipa · Pull Request #11630 · bitcoin/bitcoin

sipa · 2017-11-07T22:26:05Z

Generalize ConvertBits a bit to also be usable for the existing Base32 and Base64 convertions (rather than just for Bech32).

meshcollider · 2017-11-07T22:46:28Z

Impressive simplification, Concept ACK

fanquake · 2017-11-08T00:33:15Z

All Travis failures seem to be the same issue in signmessages.py

    self.run_test()
  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/signmessages.py", line 24, in run_test
    assert(self.nodes[0].verifymessage(address, signature, message))
  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/coverage.py", line 47, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/authproxy.py", line 138, in __call__
    raise JSONRPCException(response['error'])
test_framework.authproxy.JSONRPCException: Malformed base64 encoding (-5)
2017-11-07 23:50:01.876000 TestFramework (INFO): Stopping nodes
2017-11-07 23:50:01.877000 TestFramework.node0 (DEBUG): Stopping node

sipa · 2017-11-08T17:14:13Z

@fanquake Thanks, fixed.

promag · 2017-11-08T20:40:30Z

src/utilstrencodings.cpp

Any reason to not reserve? Doesn't pay off?

promag · 2017-11-08T22:53:24Z

src/utilstrencodings.cpp

Not sure but isn't (p - e) & 7 == 0 faster? Or does the compiler optimise this case?

Strength reduction will do that for you at -O1 and higher.

TheBlueMatt · 2017-12-08T17:18:28Z

utACK 16a01bf1a2a318effbbac4a22f1092fee1924dc8. Would be nice to add these folks to test_bitcoin_fuzzy.cpp!

laanwj · 2018-03-01T22:59:21Z

utACK. Needs rebase in src/utilstrencodings.cpp.

sipa · 2018-03-05T21:57:23Z

Rebased.

laanwj · 2018-03-05T22:19:05Z

src/utilstrencodings.cpp

It seems a bit of a pity to allocate and fill an intermediate vector here, which is just the contents of the string mapped through decode64_table. Could we maybe wrap the iterator to weave this into ConvertBits?
(to be clear, this could be done later, no need to do so in this PR, but as the rest of the changes is so elegant it just jumped out)

@laanwj I actually tried that, and it became a bit more complex than I wanted to do in this PR.

Yes it seems quite annoying to do so in C++, iterators are tricky enough to make this seemingly trivial thing non-trivial :/

So as we keep it like this, we should heed @promag's suggestion and reserve, I think.

laanwj · 2018-03-06T23:10:22Z

Needs rebase after #11372

sipa · 2018-03-07T04:30:34Z

Rebased.

sipa · 2018-03-07T14:58:21Z

Added reserves in several places.

laanwj · 2018-03-07T15:23:33Z

Added reserves in several places.

Thanks!

utACK b3ea8cc

b3ea8cc Simplify Base32 and Base64 conversions (Pieter Wuille) 3296a3b Generalize ConvertBits (Pieter Wuille) Pull request description: Generalize `ConvertBits` a bit to also be usable for the existing Base32 and Base64 convertions (rather than just for Bech32). Tree-SHA512: 3858247f9b14ca4766c08ea040a09b1d6d70caaccc75c2436a54102d6d526f499ec07f5bdfcbbe16cbde5aae521cd16e9aa693e688a97e6c5e74b8e58ee55a13

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.

Summary: The behavior of ConvertBits() has been simplified in D572 from it's original form, and would return `false` when there is any remaining bit and padding is disabled. This diff relaxes this requirement to be in par with the original behavior (see https://github.com/bitcoin/bitcoin/pull/11167/files#diff-b5877ad42a7dbee9a99cbf0596977fd9). The call will now returns `true` if the remaining bits are all zeros. This is a prerequisite for backporting [[bitcoin/bitcoin#11630 | PR11630]]. The cash address decoding is updated to now check the output of `ConvertBits()` Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5734

Summary: ``` Generalize ConvertBits a bit to also be usable for the existing Base32 and Base64 convertions (rather than just for Bech32). ``` Backport of core [[bitcoin/bitcoin#11630 | PR11630]]. Depends on D5734. The key_io.cpp changes are not ported because they are segwit related, however some cashaddrenc.cpp and util_tests.cpp changes were required. Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5735

Summary: The behavior of ConvertBits() has been simplified in D572 from it's original form, and would return `false` when there is any remaining bit and padding is disabled. This diff relaxes this requirement to be in par with the original behavior (see https://github.com/bitcoin/bitcoin/pull/11167/files#diff-b5877ad42a7dbee9a99cbf0596977fd9). The call will now returns `true` if the remaining bits are all zeros. This is a prerequisite for backporting [[bitcoin/bitcoin#11630 | PR11630]]. The cash address decoding is updated to now check the output of `ConvertBits()` Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5734

Summary: ``` Generalize ConvertBits a bit to also be usable for the existing Base32 and Base64 convertions (rather than just for Bech32). ``` Backport of core [[bitcoin/bitcoin#11630 | PR11630]]. Depends on D5734. The key_io.cpp changes are not ported because they are segwit related, however some cashaddrenc.cpp and util_tests.cpp changes were required. Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5735

b3ea8cc Simplify Base32 and Base64 conversions (Pieter Wuille) 3296a3b Generalize ConvertBits (Pieter Wuille) Pull request description: Generalize `ConvertBits` a bit to also be usable for the existing Base32 and Base64 convertions (rather than just for Bech32). Tree-SHA512: 3858247f9b14ca4766c08ea040a09b1d6d70caaccc75c2436a54102d6d526f499ec07f5bdfcbbe16cbde5aae521cd16e9aa693e688a97e6c5e74b8e58ee55a13

fanquake added the Refactoring label Nov 7, 2017

sipa force-pushed the 201711_simpleconvert branch from 927db41 to 8059735 Compare November 8, 2017 02:41

promag reviewed Nov 8, 2017

View reviewed changes

src/utilstrencodings.cpp Outdated

Copy link

Contributor

promag Nov 8, 2017 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to not reserve? Doesn't pay off?

promag reviewed Nov 8, 2017

View reviewed changes

sipa force-pushed the 201711_simpleconvert branch from 8059735 to 16a01bf Compare November 23, 2017 01:59

laanwj added this to the 0.17.0 milestone Mar 5, 2018

sipa force-pushed the 201711_simpleconvert branch from 16a01bf to eb59db5 Compare March 5, 2018 21:57

laanwj reviewed Mar 5, 2018

View reviewed changes

Generalize ConvertBits

3296a3b

sipa force-pushed the 201711_simpleconvert branch from eb59db5 to 110556f Compare March 7, 2018 04:30

sipa force-pushed the 201711_simpleconvert branch from 110556f to bc528e6 Compare March 7, 2018 14:57

Simplify Base32 and Base64 conversions

b3ea8cc

sipa force-pushed the 201711_simpleconvert branch from bc528e6 to b3ea8cc Compare March 7, 2018 15:04

laanwj merged commit b3ea8cc into bitcoin:master Mar 7, 2018

str4d mentioned this pull request Jun 7, 2018

Sapling address encodings zcash/zcash#3326

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

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

Uh oh!

Uh oh!

meshcollider commented Nov 7, 2017

Uh oh!

fanquake commented Nov 8, 2017

Uh oh!

sipa commented Nov 8, 2017

Uh oh!

promag Nov 8, 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!

promag Nov 8, 2017

Choose a reason for hiding this comment

Uh oh!

sipa Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Dec 8, 2017

Uh oh!

laanwj commented Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Mar 5, 2018

Uh oh!

laanwj Mar 5, 2018 • 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!

sipa Mar 7, 2018

Choose a reason for hiding this comment

Uh oh!

laanwj Mar 7, 2018 • 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!

laanwj commented Mar 6, 2018

Uh oh!

sipa commented Mar 7, 2018

Uh oh!

sipa commented Mar 7, 2018

Uh oh!

laanwj commented Mar 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sipa commented Nov 7, 2017 •

edited

Loading

promag Nov 8, 2017 •

edited

Loading

laanwj commented Mar 1, 2018 •

edited

Loading

laanwj Mar 5, 2018 •

edited

Loading

laanwj Mar 7, 2018 •

edited

Loading