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

feat: Poseidon2 Hash Instantiation for BLS12-377 #623

Merged
merged 22 commits into from
Feb 5, 2025
Merged

Conversation

Tabaie
Copy link
Contributor

@Tabaie Tabaie commented Feb 3, 2025

Description

This PR provides a generic Merkle Damgard construction, an instantiation of Poseidon2 as a hash for BLS12-377, and GKR gates for the same curve.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How has this been tested?

Please describe the tests that you ran or implemented to verify your changes. Provide instructions so we can reproduce.

  • Test A
  • Test B

How has this been benchmarked?

Please describe the benchmarks that you ran to verify your changes.

  • Benchmark A, on Macbook pro M1, 32GB RAM
  • Benchmark B, on x86 Intel xxx, 16GB RAM

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I did not modify files generated from templates
  • golangci-lint does not output errors locally
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@Tabaie Tabaie marked this pull request as draft February 3, 2025 19:54
@Tabaie Tabaie marked this pull request as ready for review February 3, 2025 21:47
Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

It seemed that we didn't have the diagonal matrices if T != 2,3, so I explicitly call panic for now to ensure this mode is not used. Previously we essentially multiplied by uninitialized (zero) matrix which is probably insecure.

I also refactored a bit so that we can better refer to the parameters and round constants from gnark side. I also made the seed fully deterministic as this is what the reference implementation did and imo leaves less room for making errors.

Finally, renamed some of the arguments/field names to better describe what they are (as previously we would have to check the actual implementation instead of the package documentation).

I also separated the GKR gates into a separate package so that we would only need to load it when needed. I also recommend using the registry-based approach as currently the gkr.Gates access is not synchronized which may lead to inconsistent state.

I see that you have already started another PR #628 which assumes this PR. Feel free to revert the commits but I think it could be useful for more generic usage.

@Tabaie Tabaie merged commit 10a243d into master Feb 5, 2025
5 checks passed
@Tabaie Tabaie deleted the feat/poseidon2-hash branch February 5, 2025 15:38
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