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.

Sign up for GitHub

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

Jump to bottom

ML-KEM: Improve compression and byte conversion functions #161

Merged
merged 8 commits into from
Nov 1, 2024
Merged

ML-KEM: Improve compression and byte conversion functions #161

merged 8 commits into from
Nov 1, 2024

Conversation

marsella
Copy link
Contributor

Closes #144 (assuming that #160 has already been merged).

This has two parts:

  • Cleans up the compression functions. These were correct and nicely written, I just changed names and docs.
  • Attempts to clean up bits-to-bytes conversion functions.

For the second part, I am honestly kind of confused. I thought the endianness was wrong but I tried rewriting it multiple times to make it look more like the spec and it seems correct. The KATs pass and all the internal properties pass. If anyone discerns something weird I'm doing with bit ordering I would love feedback.

mccleeary-galois
mccleeary-galois reviewed Oct 25, 2024
View reviewed changes
Copy link
Contributor

@mccleeary-galois mccleeary-galois left a comment

Choose a reason for hiding this comment

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

Caught this on this PR, but the ML-KEM proofs currently require z3 v4.13+. This is what is installed by default with brew install cryptol but it is not what we ship with what4 solvers as that is v4.8.14. Adding a ticket to ensure we do :check-docstring against multiple z3 versions. We should also consider adding support to be able to set z3 version.

@mccleeary-galois
Copy link
Contributor

Caught this on this PR, but the ML-KEM proofs currently require z3 v4.13+. This is what is installed by default with brew install cryptol but it is not what we ship with what4 solvers as that is v4.8.14. Adding a ticket to ensure we do :check-docstring against multiple z3 versions. We should also consider adding support to be able to set z3 version.

Going to cut some tickets for CI related tasks from this and also try and figure out which properties are actually causing the problem.

@mccleeary-galois mccleeary-galois mentioned this pull request Oct 31, 2024
Open
6 tasks
@mccleeary-galois
Copy link
Contributor

For the second part, I am honestly kind of confused. I thought the endianness was wrong but I tried rewriting it multiple times to make it look more like the spec and it seems correct. The KATs pass and all the internal properties pass. If anyone discerns something weird I'm doing with bit ordering I would love feedback.

Looking at the spec I agree that this is not super clear on endianness here. I am happy with how this looks right now.

mccleeary-galois
mccleeary-galois approved these changes Oct 31, 2024
View reviewed changes
Copy link
Contributor

@mccleeary-galois mccleeary-galois left a comment

Choose a reason for hiding this comment

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

LGTM, opened up a separate issue #166 for the issue uncovered during review of this.

@marsella
mlkem: tweak rounding property #144
e9f2576
@marsella
mlkem: expand docs on compression #144
0eb75d1
@marsella
mlkem: clean up properties for compression #144
9bdb16c
@marsella
mlkem: rename compression functions with types #144
9eec5ad 
Replaces tick marks with explicit denotations of what dimension the
input and output are.
@marsella
mlkem: add bit-to-byte conversion properties #144
40886cd
@marsella
mlkem: make b2b look more like spec #144
23d2d89 
Changes implementation details of BytesToBits and BitsToBytes
@marsella
mlkem: switch prove to exhaust #144
78a0899 
This is to support different z3 versions, some of which don't prove the
property in a reasonable amount of time. However since it's exhaustable
we can do that equivalently.
@marsella
mlkem: add reference to helper property #144
d50c74d
@marsella
Copy link
Contributor Author

marsella commented Nov 1, 2024

I had to make a number of picky little changes that overlapped with the encoding pr #160. Nothing very interesting.

In service of #166 I switched a few properties to exhaust instead of prove, so I believe this now works with both z3 versions (the latest 4.14 and the shipped version 4.8.14)

@marsella marsella merged commit e2cefbd into master Nov 1, 2024
2 checks passed
@marsella marsella deleted the 144-compress branch November 1, 2024 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mccleeary-galois mccleeary-galois mccleeary-galois approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bring encoding and compression functions in ML-KEM up to gold standard
2 participants
@marsella @mccleeary-galois