Skip to content
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

Replace hashlib.ripemd160 with pythonic implementation #120

Merged
merged 4 commits into from
Dec 3, 2022

Conversation

yashasvi-ranawat
Copy link
Contributor

Closes #118

@yashasvi-ranawat
Copy link
Contributor Author

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

@yashasvi-ranawat
Copy link
Contributor Author

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.

Copy link
Member

@merc1er merc1er left a comment

Choose a reason for hiding this comment

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

Thanks for this! I will set up different environments in order to test that.

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

bitcash/crypto.py Outdated Show resolved Hide resolved
@merc1er
Copy link
Member

merc1er commented Nov 27, 2022

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
Copy link
Contributor Author

yashasvi-ranawat commented Nov 27, 2022

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
Copy link
Contributor Author

yashasvi-ranawat commented Nov 27, 2022

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
Copy link
Contributor Author

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
Copy link
Member

merc1er commented Nov 28, 2022

Is this ready for final review @yashasvi-ranawat?

Copy link
Member

@merc1er merc1er left a comment

Choose a reason for hiding this comment

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

Thank you for this!

@merc1er merc1er merged commit 08dac6e into pybitcash:master Dec 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unsupported hash type ripemd160
2 participants