search

pybitcash / bitcash

BitCash: Python Bitcoin Cash Library (fork of ofek's Bit)
https://bitcash.dev
MIT License
97 stars 39 forks source link

Replace hashlib.ripemd160 with pythonic implementation #120

Closed yashasvi-ranawat closed 1 year ago

yashasvi-ranawat commented 1 year ago

Closes #118

yashasvi-ranawat commented 1 year ago

the pythonic implementation is 2 orders magnitude slower. Will only use it as fallback, in the next commit

yashasvi-ranawat commented 1 year ago

In the present application of the function, where it is only used to generate P2PKH locking script (and cashaddress), I think this is okay.

However, if in the future BitCash implements P2SH wallets, then the wallet can leak what underlying implementation it uses when asked to verify a script. Here, of course, switching to P2SH32 would be a solution, which also follows the general recommendation against using ripemd160.

merc1er commented 1 year ago

Here, of course, switching to P2SH32 would be a solution, which also follows the general recommendation against using ripemd160.

Sorry, I am not familiar with P2SH32 and the upcoming network upgrade. Is P2SH32 meant to replace P2SH or work along it? And does that meant ripemd160 will no longer be necessary?

yashasvi-ranawat commented 1 year ago

Here, of course, switching to P2SH32 would be a solution, which also follows the general recommendation against using ripemd160.

Sorry, I am not familiar with P2SH32 and the upcoming network upgrade. Is P2SH32 meant to replace P2SH or work along it? And does that meant ripemd160 will no longer be necessary?

It is meant to go along with P2SH (since a lot of contracts have used it, and a lot of wallets have it implemented). The primary idea is to limit hash collision of complex scripts, which is a possibility with P2SH.

Although, ripemd160 will still be relevant with P2PKH and P2SH. I haven't completely followed the discussion either, but it is heavily recommended for the P2SH scripts to switch to P2SH32.

yashasvi-ranawat commented 1 year ago

Could you please add unit tests to all individual functions as well as for ripemd160_sha256?

I monkeypatched tests for pythonic ripemd160_sha256. It should test it on any environment. will need further testing.

yashasvi-ranawat commented 1 year ago

Could you please add unit tests to all individual functions as well as for ripemd160_sha256?

Correct monkeypatching implemented to test pythonic ripemd160. should test it regardless of environment.

merc1er commented 1 year ago

Is this ready for final review @yashasvi-ranawat?