test: replace hashlib.ripemd160 with an own implementation #23716
Conversation
sipa
force-pushed
the
202112_ripemd160
branch
from
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.
What makes this inherently test-only?
There was a problem hiding this comment.
IMO should be mentioned in the comment.
There was a problem hiding this comment.
That's indeed a good warning to add in a followup.
There was a problem hiding this comment.
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.
@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. |
maflcko
left a comment
maflcko
left a comment
There was a problem hiding this comment.
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.
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 ?
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.
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.
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) ])
TrueThere was a problem hiding this comment.
Yes, the result of & is only negative if both inputs are negative.
|
Thanks for this! I've ported to python 2 and integrated into pycoin here richardkiss/pycoin@6b3e4bb |
9 participants
Closes #23710.