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

Functional correctness for portable encoding commitment #776

Merged
merged 15 commits into from
Feb 3, 2025

Conversation

W95Psp
Copy link
Contributor

@W95Psp W95Psp commented Jan 29, 2025

This PR adds a specification and a proof for the serialize function of simd/portable/encoding/commitment in ML-DSA.

To do so, I had to take split the function in many smaller functions.

Fixes #745.

@W95Psp W95Psp requested a review from a team as a code owner January 29, 2025 16:48
Copy link
Member

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

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

Only a few nits and questions

libcrux-ml-dsa/src/simd/portable/encoding/commitment.rs Outdated Show resolved Hide resolved
libcrux-ml-dsa/src/simd/portable/encoding/commitment.rs Outdated Show resolved Hide resolved
libcrux-ml-dsa/src/simd/portable/encoding/commitment.rs Outdated Show resolved Hide resolved
libcrux-ml-dsa/src/simd/portable/encoding/commitment.rs Outdated Show resolved Hide resolved
libcrux-ml-dsa/src/simd/portable/encoding/commitment.rs Outdated Show resolved Hide resolved
libcrux-ml-dsa/src/simd/portable/encoding/commitment.rs Outdated Show resolved Hide resolved
libcrux-ml-dsa/src/simd/portable/encoding/commitment.rs Outdated Show resolved Hide resolved
@franziskuskiefer
Copy link
Member

I filed AeneasVerif/eurydice#138 for the C failure. Unfortunately, I don't see a workaround for this right now.

@W95Psp W95Psp force-pushed the lf-portable-encoding-commitment branch from 625997e to ec0a126 Compare January 30, 2025 07:47
@karthikbhargavan
Copy link
Contributor

I opened an issue for follow-up improvements: #777
Let's add to that as we review and merge this PR.

@karthikbhargavan
Copy link
Contributor

@W95Psp I adapted the code and changed the proof accordingly.
Please review and then: address the z3 version issue on CI (likely pinning to 4.13.3 would work)

I have no idea what the build failures with address sanitizer are doing on macos-latest.

Newer releases of F* now include both z3 4.8.5 and 4.13.3
@W95Psp
Copy link
Contributor Author

W95Psp commented Feb 3, 2025

Seems OK to me, though we should do something about the proofs: already my calc thing is too big for such a small function, now with the asserts that's a bit overwhelming.
But that works.

For Z3, I updated the job to use latest F*, which ships with Z3 4.13.3.

@W95Psp
Copy link
Contributor Author

W95Psp commented Feb 3, 2025

I experimented a bit with unrolling loops @karthikbhargavan, in branch lf-portable-encoding-commitment-experiment-unroll.

Sadly, this doesn't work out of the box, because of subslices.
I tried to define locally []. and update slice to have quantifiers everywhere, but that doesn't help.
Shall I investigate more?

@karthikbhargavan
Copy link
Contributor

This PR still has an ASAN failure on Rust nightly on macos-latest.
However, I am going to merge it and do any necessary fixes in a future PR.

@karthikbhargavan karthikbhargavan merged commit cd46c47 into main Feb 3, 2025
59 of 62 checks passed
@karthikbhargavan karthikbhargavan deleted the lf-portable-encoding-commitment branch February 3, 2025 12:24
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.

Verify ML-DSA: portable/encoding/commitment
3 participants