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

Septidon integration #14

Merged
merged 6 commits into from
Mar 21, 2023
Merged

Septidon integration #14

merged 6 commits into from
Mar 21, 2023

Conversation

naure
Copy link
Contributor

@naure naure commented Mar 16, 2023

Integrate the septuple implementation of the permutation with the sponge/lookup circuit.

The original implementation with double rounds is still supported. The same tests run with both implementations. The implementation of the new shorter permutation is selected by a feature short like so in Cargo.toml:

poseidon-circuit = { git = "https://github.com/scroll-tech/poseidon-circuit.git", features = ["short"] }

This is built on top of #12. It might be easier to review the new chip in #12, then review the integration here (#14).

Copy link
Member

@noel2004 noel2004 left a comment

Choose a reason for hiding this comment

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

  1. I guess we can reorganize some modules, for example: pick some Spec relatied struct like P128Pow5T3 out of poseidon module to avoid an explicit exposing some of them. Or just put the whole module of septidon under poseidon since both it and pow5 have implied a PermuteChip trait.

  2. It should be noted that the new PR has broken the code in zkevm-circuit and mpt which use PoseidonHashConfig and PoseidonHashChip. What about making an additional wrapping for these structures and use a feature to select the preferred permute chip?

@noel2004
Copy link
Member

Again I would merge first so #13 can process. Later we should try to resolve the cmpability.

@noel2004 noel2004 merged commit bd92043 into main Mar 21, 2023
@naure naure mentioned this pull request Mar 21, 2023
@naure
Copy link
Contributor Author

naure commented Mar 21, 2023

Thanks for the review! I have applied your suggestions in #16 .

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