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
test: replace hashlib.ripemd160 with an own implementation #23716
Conversation
cdfaa53
to
5b559dc
Compare
ACK 5b559dc, pending CI Verified test vectors match my system's ripemd160 installation. Did not review the actual crypto. |
# Copyright (c) 2021 Pieter Wuille | ||
# Distributed under the MIT software license, see the accompanying | ||
# file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
"""Test-only pure Python RIPEMD160 implementation.""" |
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.
What makes this inherently test-only?
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.
It's not constant time.
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.
IMO should be mentioned in the comment.
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.
That's indeed a good warning to add in a followup.
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.
It's not constant time.
Given that its only use in Bitcoin is ripemd160(sha256(data))
, does this not make a timing attack effectively useless?
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.
@mmgen Yes, but it's open-source code, and open-source code gets copied and can end up being adopted for other purposes.
After merge there will be one other remaining use of Going to merge this for our CI, but I plan to review after merge. |
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.
Looks good and given that there are tests, this is probably correct. Left a nit to show that I read the code.
for b in range(len(data) >> 6): | ||
state = compress(*state, data[64*b:64*(b+1)]) | ||
# Construct final blocks (with padding and size). | ||
pad = b"\x80" + b"\x00" * ((119 - len(data)) & 63) |
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: using the &
operator is a slightly odd way to change the sign of an integer, no?
Might be better written as (119 - len&63) & 63 ?
See also https://docs.python.org/3/library/stdtypes.html#bitwise-operations-on-integer-types
Performing these calculations with at least one extra sign extension bit in a finite two’s complement representation (a working bit-width of 1 + max(x.bit_length(), y.bit_length()) or more) is sufficient to get the same result as if there were an infinite number of sign bits.
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.
I'm happy to change it, but "& 63" is exactly the same as "% 64" but faster, and that should be sufficient here.
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.
Oh it is fine. I was just commenting that &63
also changes the sign from negative to positive.
For example if the size is 172
, then 119-172
will be negative and &63
will make it positive.
Edit: I checked that the two do the same:
>>> all([ (119-i)&63==(119-i&63)&63 for i in range(1000000) ])
True
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.
Yes, the result of &
is only negative if both inputs are negative.
…ementation 5b559dc Swap out hashlib.ripemd160 for own implementation (Pieter Wuille) ad3e9e1 Add pure Python RIPEMD-160 (Pieter Wuille) Pull request description: Closes bitcoin#23710. ACKs for top commit: jamesob: ACK bitcoin@5b559dc, pending CI Tree-SHA512: dcd4ea2027eac572f7ab0da434b081b9a5d6b78675e559258a446b4d254b29d93c4d2cc12da4a28303543d6d99f5f2246fde4052e84af81d18e04399b137b39e
Github-Pull: bitcoin#23716 Rebased-From: ad3e9e1
Github-Pull: bitcoin#23716 Rebased-From: 5b559dc
…own implementation 5238614 Swap out hashlib.ripemd160 for own implementation (Pieter Wuille) 9a0a924 Add pure Python RIPEMD-160 (Pieter Wuille) Pull request description: Closes #23710. ACKs for top commit: jamesob: ACK bitcoin/bitcoin@5238614, pending CI Tree-SHA512: dcd4ea2027eac572f7ab0da434b081b9a5d6b78675e559258a446b4d254b29d93c4d2cc12da4a28303543d6d99f5f2246fde4052e84af81d18e04399b137b39e
Summary: OpenSSL 3 removed `ripemd160`, see [[openssl/openssl#16994 | openssl/openssl#16994]] Thus, the functional tests fail on systems with OpenSSL 3. This is a backport of [[bitcoin/bitcoin#23716 | core#23716]] Test Plan: `ninja check-functional-extended` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11413
…ementation 5b559dc Swap out hashlib.ripemd160 for own implementation (Pieter Wuille) ad3e9e1 Add pure Python RIPEMD-160 (Pieter Wuille) Pull request description: Closes bitcoin#23710. ACKs for top commit: jamesob: ACK bitcoin@5b559dc, pending CI Tree-SHA512: dcd4ea2027eac572f7ab0da434b081b9a5d6b78675e559258a446b4d254b29d93c4d2cc12da4a28303543d6d99f5f2246fde4052e84af81d18e04399b137b39e
* Fix build of qtbase in contrib for Gcc 11.x It adds a patch with missing include <limits> in qtbase/src/tools/moc/generator.cpp * Merge bitcoin#23716: test: replace hashlib.ripemd160 with an own implementation 5b559dc Swap out hashlib.ripemd160 for own implementation (Pieter Wuille) ad3e9e1 Add pure Python RIPEMD-160 (Pieter Wuille) Pull request description: Closes bitcoin#23710. ACKs for top commit: jamesob: ACK bitcoin@5b559dc, pending CI Tree-SHA512: dcd4ea2027eac572f7ab0da434b081b9a5d6b78675e559258a446b4d254b29d93c4d2cc12da4a28303543d6d99f5f2246fde4052e84af81d18e04399b137b39e * Updates doc for Unix build: added missing dependency bison Co-authored-by: MarcoFalke <falke.marco@gmail.com>
Thanks for this! I've ported to python 2 and integrated into pycoin here richardkiss/pycoin@6b3e4bb |
* Fix build of qtbase in contrib for Gcc 11.x It adds a patch with missing include <limits> in qtbase/src/tools/moc/generator.cpp * Merge bitcoin#23716: test: replace hashlib.ripemd160 with an own implementation 5b559dc Swap out hashlib.ripemd160 for own implementation (Pieter Wuille) ad3e9e1 Add pure Python RIPEMD-160 (Pieter Wuille) Pull request description: Closes bitcoin#23710. ACKs for top commit: jamesob: ACK bitcoin@5b559dc, pending CI Tree-SHA512: dcd4ea2027eac572f7ab0da434b081b9a5d6b78675e559258a446b4d254b29d93c4d2cc12da4a28303543d6d99f5f2246fde4052e84af81d18e04399b137b39e * Updates doc for Unix build: added missing dependency bison Co-authored-by: MarcoFalke <falke.marco@gmail.com>
* Fix build of qtbase in contrib for Gcc 11.x It adds a patch with missing include <limits> in qtbase/src/tools/moc/generator.cpp * Merge bitcoin#23716: test: replace hashlib.ripemd160 with an own implementation 5b559dc Swap out hashlib.ripemd160 for own implementation (Pieter Wuille) ad3e9e1 Add pure Python RIPEMD-160 (Pieter Wuille) Pull request description: Closes bitcoin#23710. ACKs for top commit: jamesob: ACK bitcoin@5b559dc, pending CI Tree-SHA512: dcd4ea2027eac572f7ab0da434b081b9a5d6b78675e559258a446b4d254b29d93c4d2cc12da4a28303543d6d99f5f2246fde4052e84af81d18e04399b137b39e * Updates doc for Unix build: added missing dependency bison Co-authored-by: MarcoFalke <falke.marco@gmail.com>
Github-Pull: bitcoin#23716 Rebased-From: ad3e9e1
Github-Pull: bitcoin#23716 Rebased-From: 5b559dc
…ementation bf79f08 Swap out hashlib.ripemd160 for own implementation (Pieter Wuille) 6bfa0be Add pure Python RIPEMD-160 (Pieter Wuille) Pull request description: Backports #23716 to 0.21. Closes #25534. Top commit has no ACKs. Tree-SHA512: d2a175d781b30249b14488b818720554995a71cae1861c7443be120a01fd7828737949ece5fc9c193a154e06f4e37d373e44ec7d9dcd64f8e86c0429abe70bb6
3a5ec11 remove openssl fallback for ripemd160, always use pure-python implementation (windsok) 27fa0e8 Add fallback to pure-python ripemd160 (windsok) Pull request description: Newer versions of OpenSSL have removed support for `ripemd160` by default which results in an `ValueError: unsupported hash type ripemd160` exception being raised any time the `Hash160` function is used within `python-bitcoinlib`. See bitcoin/bitcoin#23710 for further details of the issue. This PR adds a fallback to a pure-python implementation of ripemd160. The code is taken from bitcoin/bitcoin#23716, and the implementation here is based on diybitcoinhardware/embit@2e293b0 Top commit has no ACKs. Tree-SHA512: f4cdbc413e262d6c9dd1692b57743c57e217f90dbf7c3ce699a8f1dfacb86ba422d7990098e623a37b2512b307800aac7ee7614d8e31e568c769899730089da8
Summary: OpenSSL 3 removed `ripemd160`, see [[openssl/openssl#16994 | openssl/openssl#16994]] Thus, the functional tests fail on systems with OpenSSL 3. This is a backport of [[bitcoin/bitcoin#23716 | core#23716]] Test Plan: `ninja check-functional-extended` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11413
Summary: OpenSSL 3 removed `ripemd160`, see [[openssl/openssl#16994 | openssl/openssl#16994]] Thus, the functional tests fail on systems with OpenSSL 3. This is a backport of [[bitcoin/bitcoin#23716 | core#23716]] Test Plan: `ninja check-functional-extended` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11413 Co-authored-by: Pieter Wuille <pieter@wuille.net>
…tion 8cd9155 Swap out hashlib.ripemd160 for own implementation (Pieter Wuille) cae0315 Add pure Python RIPEMD-160 (Pieter Wuille) Pull request description: OpenSSL 3.x removed support for ripemd160, which is what python's hashlib library was using to provide this feature. As such, systems that ship with or have otherwise installed OpenSSL 3.x (Ubuntu 22.04+ for example) will encounter functional test failures. We only use hashlib in functional tests specifically for ripemd160, so can just replace the dependency with a native implementation of ripemd160. Ref: bitcoin#23716, bitcoin#23710 ACKs for top commit: panleone: tACK 8cd9155 tests are locally working on my pc Liquid369: tACK 8cd9155 Tree-SHA512: 0536d064360f9cdad16bb6dcc92020f303a94bd6ce123a9d7dbbb8f366a75e09499240f1e37caaaffa5b82d6b6793a69029a4f15ca7ad99710046122efb8ff98
…e implementation 8cd9155 Swap out hashlib.ripemd160 for own implementation (Pieter Wuille) cae0315 Add pure Python RIPEMD-160 (Pieter Wuille) Pull request description: OpenSSL 3.x removed support for ripemd160, which is what python's hashlib library was using to provide this feature. As such, systems that ship with or have otherwise installed OpenSSL 3.x (Ubuntu 22.04+ for example) will encounter functional test failures. We only use hashlib in functional tests specifically for ripemd160, so can just replace the dependency with a native implementation of ripemd160. Ref: bitcoin#23716, bitcoin#23710 ACKs for top commit: panleone: tACK 8cd9155 tests are locally working on my pc Liquid369: tACK 8cd9155 Tree-SHA512: 0536d064360f9cdad16bb6dcc92020f303a94bd6ce123a9d7dbbb8f366a75e09499240f1e37caaaffa5b82d6b6793a69029a4f15ca7ad99710046122efb8ff98
Currently test runner doesn't execute the unit tests of the ripemd160 module, so add it to the list. All other framework modules that contain unit tests are included, as can be easily checked via `git grep unittest.TestCase ./test/functional/test_framework/` This is a late follow-up to PR bitcoin#23716 (commit ad3e9e1).
Currently test runner doesn't execute the unit tests of the ripemd160 module, so add it to the list. All other framework modules that contain unit tests are included, as can be easily checked via `git grep unittest.TestCase ./test/functional/test_framework/` This is a late follow-up to PR bitcoin#23716 (commit ad3e9e1).
82e6e3c test: add ripemd160 to test framework modules list (Sebastian Falbesoner) Pull request description: Currently test runner doesn't execute the unit tests of the ripemd160 module, so add it to the list. All other framework modules that contain unit tests are included, as can be easily checked via `$ git grep unittest.TestCase ./test/functional/test_framework/` This is a late follow-up to PR #23716 (commit ad3e9e1). ACKs for top commit: MarcoFalke: lgtm ACK 82e6e3c Tree-SHA512: 10940e215f728291c7149931a356bfc42795c098bda76d760dfa68f86443a3755e1cd35cb9a8a7b2f48880beb53f3bee3842de2d74bcadd45c7b05c13ff04203
82e6e3c test: add ripemd160 to test framework modules list (Sebastian Falbesoner) Pull request description: Currently test runner doesn't execute the unit tests of the ripemd160 module, so add it to the list. All other framework modules that contain unit tests are included, as can be easily checked via `$ git grep unittest.TestCase ./test/functional/test_framework/` This is a late follow-up to PR bitcoin#23716 (commit ad3e9e1). ACKs for top commit: MarcoFalke: lgtm ACK 82e6e3c Tree-SHA512: 10940e215f728291c7149931a356bfc42795c098bda76d760dfa68f86443a3755e1cd35cb9a8a7b2f48880beb53f3bee3842de2d74bcadd45c7b05c13ff04203
Currently test runner doesn't execute the unit tests of the ripemd160 module, so add it to the list. All other framework modules that contain unit tests are included, as can be easily checked via `git grep unittest.TestCase ./test/functional/test_framework/` This is a late follow-up to PR bitcoin#23716 (commit ad3e9e1).
Closes #23710.