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

bug: possibly incorrect DST_prime in ExpandMsgXmd #477

Closed
hussein-aitlahcen opened this issue Jan 23, 2024 · 6 comments · Fixed by #478
Closed

bug: possibly incorrect DST_prime in ExpandMsgXmd #477

hussein-aitlahcen opened this issue Jan 23, 2024 · 6 comments · Fixed by #478

Comments

@hussein-aitlahcen
Copy link
Contributor

Unless I am missing something, it look like instead of DST_prime = I2OSP(len(DST), 1) ∥ DST, we do DST_prime = DST || I2OSP(len(DST), 1) in:

@ivokub
Copy link
Collaborator

ivokub commented Jan 23, 2024

Indeed, I also encountered it some time ago. I'd have to recheck, but imo it was due to different versions of the RFC we implemented hash-to-field from.

@hussein-aitlahcen
Copy link
Contributor Author

Indeed, I also encountered it some time ago. I'd have to recheck, but imo it was due to different versions of the RFC we implemented hash-to-field from.

I figured out while rewriting this in a gnark circuit... Do you want me to open a PR or we consider this to be fine (maybe backward compatibility would be hurt here)?

@ivokub
Copy link
Collaborator

ivokub commented Jan 23, 2024

So, we have referenced version 06 in the code, but actually implemented the final version, see: https://datatracker.ietf.org/doc/html/rfc9380.

@ivokub
Copy link
Collaborator

ivokub commented Jan 23, 2024

Indeed, it would already hurt backwards compatibility as we also implement hash-to-field in the Solidity smart contract verifier and the current PLONK version we have is audited, so cannot modify easily anymore.

@hussein-aitlahcen
Copy link
Contributor Author

Ah the implementation is correct then, let me just change the reference in case anyone hit this again haha. Thanks for the quick answer!

@ivokub
Copy link
Collaborator

ivokub commented Jan 23, 2024

It would be perfect!

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 a pull request may close this issue.

2 participants