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

Use pysha3 on python<3.9 #14

Merged
merged 1 commit into from
Jun 11, 2023
Merged

Use pysha3 on python<3.9 #14

merged 1 commit into from
Jun 11, 2023

Conversation

xloem
Copy link

@xloem xloem commented Jun 4, 2023

Looks like safe-pysha3 is only compatible with python3.9 and above, so I added specifiers to select the dependency depending on python version.

@ctrl-Felix
Copy link
Owner

Thank you for suggesting this change.

To me it seems like the pysha3 package is only supported for python 2.7 - python 3.5 which would still mean it won't work on 3.6, 3.7 and 3.8.

The whole issue about this is that Ethereum uses KECCAK256 as hashing algorithm (https://ethereum.stackexchange.com/a/554) which has been replaced by SHA3 in 2015. And therefore it is actually quite hard to maintain.

I am not sure how web3py handles keccak. But I remember that it was one of the hardest challenges I faced when adding Evmos and EVM support.

What are your thoughts on this. Maybe pysha3 works also for python3.5+, do you know that?

@xloem
Copy link
Author

xloem commented Jun 8, 2023

  • Both libraries appear to support the original keccak algorithm
  • pysha3’s last release was in February of 2017, which is before python 3.6 was released. This is why it only mentions python 3.5 .
  • safe-pysha3 made two changes to pysha3, which are listed at tiran/pysha3@master...5afe:pysha3:master :
    1. it forward-ported the code to python 3.11 by changing a type symbol, i briefly found this mentioned at Request for PEP 387 exception: Py_SET_TYPE python/steering-council#79 . this broke compatibility with python 3.8 and below.
    2. it addressed a security vulnerability with processing data larger than 2^32 - 200 bytes.

So, assuming all data is not that big, the change looks okay now, but if there are further security fixes they will not be integrated which is unfortunate.

@ctrl-Felix
Copy link
Owner

Alright. Thanks for providing further clarification. I will merge it then.

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.

2 participants