fix: poseidon2 compression didn't include feed-forward#1457
Closed
fix: poseidon2 compression didn't include feed-forward#1457
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the Poseidon2 compression function by including the missing feed-forward component, ensuring that the behavior between gnark and gnark-crypto is consistent. It also adds an example test for using non-default parameters to validate instance generation between the two libraries.
- Fixed compression logic in Poseidon2 to add the feed-forward value.
- Added a custom parameter test to validate implementation consistency.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| std/permutation/poseidon2/poseidon2.go | Updated Compress to add the feed-forward during compression. |
| std/hash/poseidon2/poseidon2_test.go | Added a test (TestPoseidon2Custom) to verify non-default parameter behavior. |
Contributor
|
#1442 covers this. |
Collaborator
Author
|
Fixed in #1442 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The compression function of Poseidon2 permutation differs between gnark and gnark-crypto. gnark-crypto includes right side feed-forward in the result but gnark not. This made the results differ.
Also added example test for non-default parameters to see that instance generation matches between gnark and gnark-crypto
Strangely - this was tested before but CI didn't fail? I don't see any obvious reasons, @gbotrel maybe you have ideas? I think it is somehow related which curves we test - currently the test was only for BLS12-377 as we don't have default parameters for other curves yet, but I think that for some test configuration it completely discarded testing.
Type of change
How has this been tested?
Added new test. Current test works locally. Strangely the test should have had failed in CI before, but didn't?
Checklist:
golangci-lintdoes not output errors locally