-
Notifications
You must be signed in to change notification settings - Fork 158
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
Ability to set seed, number of rounds and exponent in MIMC hash #221
Comments
Hi @shryasss , where does the 7 come from? Normally for BN254, the smallest permutation is x -> x^5. Ok for a PR to change the number of rounds, etc. But one advantage of hard coding the exponent is that the permutation is slightly more efficient because we don't need to do a double&add algo but rather we can directly write the shortest addition chain. |
So you are saying that having exponent 5 is efficient? but I have seen some libraries for solidity which use the mimc7 so I thought that should also be possible using gnark. Even iden3 has a mimc7 implementation. you can check it here - https://github.com/iden3/go-iden3-crypto/blob/master/mimc7/mimc7.go |
@shryasss parametrizing MiMC could make sense. If you want to take a shot at it, let me know. I think it's a good opportunity to experiment with generic. Recently in Which enables a clean usage across in-circuit / out-of-circuit:
Which would translate similarly to |
Hey @gbotrel! I would love to take this up and make a PR, please let me know on how to proceed on this. |
Referencing to https://github.com/ConsenSys/gnark-crypto/blob/master/ecc/bn254/fr/mimc/mimc.go
There should be a way to set mimcNbRounds, seed and exponent (which is currently set to 5).
I find that this modularity would help us get more control over the hash.
I was using the snark to verify on ethereum and the mimc implementation on solidity was using a different seed and exponent value, so I had to change it manually in the go modules which I think is not a great way to do things.
If I get an idea of how I should implement this issue, I would love to make a PR to solve this.
Thanks!
The text was updated successfully, but these errors were encountered: