test: replace hashlib.ripemd160 with an own implementation #23716

pull sipa wants to merge 2 commits into bitcoin:master from sipa:202112_ripemd160 changing 2 files +133 −2

sipa commented at 7:19 PM on December 8, 2021: member

Closes #23710.
Add pure Python RIPEMD-160 ad3e9e1f21
Swap out hashlib.ripemd160 for own implementation 5b559dc7ec
sipa force-pushed on Dec 8, 2021
laanwj added the label Tests on Dec 8, 2021
jamesob commented at 7:48 PM on December 8, 2021: member

ACK https://github.com/bitcoin/bitcoin/pull/23716/commits/5b559dc7ecf37ab1604b75ec8ffe8436377a5fb1, pending CI

Verified test vectors match my system's ripemd160 installation. Did not review the actual crypto.
in test/functional/test_framework/ripemd160.py:4 in 5b559dc7ec
```
   0 | @@ -0,0 +1,130 @@
   1 | +# Copyright (c) 2021 Pieter Wuille
   2 | +# Distributed under the MIT software license, see the accompanying
   3 | +# file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 | +"""Test-only pure Python RIPEMD160 implementation."""
```
luke-jr commented at 2:45 AM on December 9, 2021:

What makes this inherently test-only?

sipa commented at 2:54 AM on December 9, 2021:

It's not constant time.

luke-jr commented at 3:06 AM on December 9, 2021:

IMO should be mentioned in the comment.

Sjors commented at 8:55 AM on December 9, 2021:

That's indeed a good warning to add in a followup.

mmgen commented at 1:37 PM on May 3, 2022:

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?

sipa commented at 1:39 PM on May 3, 2022:

@mmgen Yes, but it's open-source code, and open-source code gets copied and can end up being adopted for other purposes.
MarcoFalke commented at 8:17 AM on December 9, 2021: member

After merge there will be one other remaining use of new(). (git grep 'hashlib.new'). Since it is discouraged in the python docs, we should replace that with the named constructor.

Going to merge this for our CI, but I plan to review after merge.
MarcoFalke merged this on Dec 9, 2021
MarcoFalke closed this on Dec 9, 2021
in test/functional/test_framework/ripemd160.py:103 in ad3e9e1f21 outdated
```
  98 | +    state = (0x67452301, 0xefcdab89, 0x98badcfe, 0x10325476, 0xc3d2e1f0)
  99 | +    # Process full 64-byte blocks in the input.
 100 | +    for b in range(len(data) >> 6):
 101 | +        state = compress(*state, data[64*b:64*(b+1)])
 102 | +    # Construct final blocks (with padding and size).
 103 | +    pad = b"\x80" + b"\x00" * ((119 - len(data)) & 63)
```
MarcoFalke commented at 2:37 PM on December 9, 2021:

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.

sipa commented at 2:43 PM on December 9, 2021:

I'm happy to change it, but "& 63" is exactly the same as "% 64" but faster, and that should be sufficient here.
MarcoFalke commented at 2:50 PM on December 9, 2021:

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
sipa commented at 2:52 PM on December 9, 2021:

Yes, the result of & is only negative if both inputs are negative.
MarcoFalke commented at 2:38 PM on December 9, 2021: member

Looks good and given that there are tests, this is probably correct. Left a nit to show that I read the code.
sidhujag referenced this in commit 8fe77bcc20 on Dec 10, 2021
luke-jr referenced this in commit 67c9c0732c on Dec 14, 2021
luke-jr referenced this in commit bd9a6fdcc4 on Dec 14, 2021
RandyMcMillan referenced this in commit 812a8998dd on Dec 23, 2021
Fabcien referenced this in commit d5df49bfeb on May 5, 2022
knst referenced this in commit 2f38f567b9 on May 20, 2022
PastaPastaPasta referenced this in commit e4dbd22532 on May 29, 2022
richardkiss commented at 10:15 PM on May 29, 2022: contributor

Thanks for this! I've ported to python 2 and integrated into pycoin here https://github.com/richardkiss/pycoin/commit/6b3e4bb3eeba4c41c33816533f7c52bbae7a6142
UdjinM6 referenced this in commit 1233977a0f on May 30, 2022
PastaPastaPasta referenced this in commit 1babc5abff on May 30, 2022
fanquake referenced this in commit 6bfa0bef48 on Jul 4, 2022
fanquake referenced this in commit bf79f08d97 on Jul 4, 2022
MarcoFalke referenced this in commit ec0a4ad677 on Jul 6, 2022
fanquake commented at 8:34 AM on July 6, 2022: member

Backported to 0.21 in #25538. 22.0 in #25250.
petertodd referenced this in commit de5c6a058c on Jul 29, 2022
PiRK referenced this in commit 8be29f310e on Aug 16, 2022
schancel referenced this in commit b30813428c on Aug 17, 2022
Fuzzbawls referenced this in commit 0d779aa53d on Apr 4, 2023
mo-bay referenced this in commit 5218ea5774 on Apr 10, 2023
theStack referenced this in commit 768ae178af on Apr 29, 2023
theStack referenced this in commit 82e6e3cae5 on May 2, 2023
fanquake referenced this in commit cfe5da4c90 on May 2, 2023
sidhujag referenced this in commit 8d1c5b5275 on May 4, 2023
RandyMcMillan referenced this in commit c6f443d62d on May 27, 2023
delta1 referenced this in commit 1413ea774b on Jun 6, 2023
DrahtBot locked this on Jul 6, 2023