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

Fix poseidon sponge bug #148

Merged
merged 2 commits into from
Oct 11, 2024
Merged

Conversation

kilic
Copy link
Contributor

@kilic kilic commented Aug 30, 2024

Description

Poseidion sponge skips a permutation in a certain condition:

  • Sponge is in squeeze mode
  • Number of elements in the call equals to the rate

So that it returns same elements in subsequent calls which is demonstrated in the first commit of this PR. Problem is fixed in the next commit. I also added a simpler independent implementation to apply cross fuzz test and must be removed when this PR matures. I've checked and asked around if any production project uses PoseidonSponge at the moment but don't see any and I think this PR is safe to be opened.

I also come across another strange behaviour which I feel that it is not intended. In a such condition:

  • Sponge is in squeeze mode
  • Number of elements int the call equals to zero
  • Internal index is factor of rate but not zero

It applies a useless permutation rather than jsut returning an empty output vector. I've also filed this it in the comments of cross implementation.

Another thing I would like to add as discussion is that squeezing 0 elements in absorb mode changes the mode to squeeze. So for example output of absorb(3), absorb(4), squeeze(1) is not the same as absorb(6), squeeze(0), absorb(1), squeeze(1) This would be intended but I find allowing to squueze 0 elements confusing as an api user.


  • Targeted PR against correct branch (main)
  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

cross test with independent implementation
@kilic kilic requested a review from a team as a code owner August 30, 2024 12:20
@kilic kilic requested review from Pratyush, mmagician and weikengchen and removed request for a team August 30, 2024 12:20
Copy link
Member

@Pratyush Pratyush left a comment

Choose a reason for hiding this comment

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

Thank you for the fix!

@Pratyush Pratyush added this pull request to the merge queue Oct 11, 2024
Merged via the queue into arkworks-rs:main with commit 8fbf5ef Oct 11, 2024
5 checks passed
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