test: clean up templates and add interop for poseidon2 babybear against plonky3 impl #661
+769
−355
Conversation
This file contains 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
There was a problem hiding this comment.
Pull Request Overview
This PR cleans up template code and updates test parameters while adding interop support for the Poseidon2 babybear implementation against Plonky3. Key changes include:
- Adding a new interop test (plonky3_interop_test.go) that reads CSV test vectors.
- Updating permutation parameters and test loops (using range iteration) in babybear, koalabear, and goldilocks test files.
- Refining generator template logic by introducing a new DiagInternal field and adjusting parameter assignments.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| field/babybear/poseidon2/plonky3_interop_test.go | Adds interop test for Poseidon2 babybear with CSV-based test vectors. |
| field/babybear/poseidon2/poseidon2_test.go | Updates permutation parameters, error messages, and loop iterations. |
| field/goldilocks/poseidon2/poseidon2_test.go | Adjusts test parameters and uses range-based loops for improved readability. |
| field/generator/generator_poseidon2.go | Refactors parameter setup using a switch and new Params field. |
| field/koalabear/poseidon2/poseidon2_test.go | Updates test parameters and loop constructions. |
| field/babybear/poseidon2/hash.go | Updates parameter values and refines comments for clarity. |
| field/goldilocks/poseidon2/hash.go | Reworks diag arrays as slices and updates related comments. |
| field/koalabear/poseidon2/hash.go | Updates diag arrays and comments to reflect the new parameter values. |
| field/generator/asm/amd64/build.go | Introduces DiagInternal in the Poseidon2Parameters struct. |
yelhousni
approved these changes
Mar 12, 2025
There was a problem hiding this comment.
looks good to me! it matches plonky3/horizen and code generation is cleaner :)
I ran the sage script from Hoizen and and it output the same constants you have in this PR. So we can also cross check koala bear. Here is the corresponding rust file: https://gist.github.com/yelhousni/4ebb017ed55425357c188be3a11b5a64
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.