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

ML-KEM: Try to clean up the NTT implementations #156

Merged
merged 8 commits into from
Nov 5, 2024
Merged

ML-KEM: Try to clean up the NTT implementations #156

merged 8 commits into from
Nov 5, 2024

Conversation

marsella
Copy link
Contributor

@marsella marsella commented Oct 16, 2024

Does not quite finish #147.

This addresses most of the clean-up issues in the linked issue, except for cleaning up the naive NTT version described in the spec (see Algorithms 9 and 10). That has a dense nested loop structure whose bounds depend on each other's variables and that iteratively updates the state of the f variable. We have a version that works but doesn't clearly map onto the spec, and I spent a day failing to find a more obviously-matched way to implement it.
Update: I made a recursive version of Algorithm 9 which may or may not be better. I solicit opinions from reviewers on whether they like it or not.

Everything else should be addressed in a reasonable way. Looking for general gold-standard quality review.

@marsella
Copy link
Contributor Author

I revisited this today and have not gained any insight in the interim week. The innermost loop of NTT accesses and updates 2 * len elements of the loop, where len is variable based on previous loop iterations. I was hoping I could try to fold across the inner loop, but ran into several problems:

  • if the iterator j is a variable, I don't think there's a way to update a single element in the output list (everything I can imagine uses take and drop to preserve the unchanged elements, which would require j to be a type)
  • There's no way to "build up" the output list as we go because the fold function has to be the same for every iteration, which means it can't be parameterized by an increasing length parameter.

I keep ending up in the following train of thought:

  • If I want to update elements of the list one-by-one, the index of them has to be a type
  • If the index is a type I'm going to have to use recursion to update it as we iterate

There might be a somewhat cleaner way to implement the recursive loops than what I did in e742abb and 0b355c3, but I think that's going to have to be the basic structure, which means it's going to be hard to read. Unless we can find a different NIST-standardized NTT description that's stated to be equivalent to this one, and can implement that instead.

Anyway, for now I propose we take the clean-up commits through b56844e and I'll spin out a separate issue to implement NTT itself more closely to the spec.

@marsella
Copy link
Contributor Author

marsella commented Oct 31, 2024

I reverted deleted my attempt at a recursive version and made #163 to handle fixing the NTT implementation.

This doesn't replace all uses of `Z_q_256`, but it gets all the easy
ones.
This replaces `'`s with suffixes explictly describing what type of data
each NTT function operates over.
This adds some documentation around the NTT module explaining where the
spec says it's allowed to choose any version of their algorithms that
are the same.
Several properties didn't have correct `repl` commands in the
docstrings.
- Adds docs to BitRev and contains its behavior a bit better
- Adjust spacing, naming, etc in MultiplyNTTs and BaseCaseMultiply
This doesn't make them spec adherent but it simplifies the section a bit.
@mccleeary-galois
Copy link
Contributor

There might be a somewhat cleaner way to implement the recursive loops than what I did in e742abb and 0b355c3, but I think that's going to have to be the basic structure, which means it's going to be hard to read. Unless we can find a different NIST-standardized NTT description that's stated to be equivalent to this one, and can implement that instead.

FWIW I found the recursive start you had here easier to follow. I was able to see how the list comprehensions you have currently still fit the spec.

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.

Looks like a good step in the right direction to me!

@marsella marsella merged commit a5b6866 into master Nov 5, 2024
2 checks passed
@marsella marsella deleted the 147-ntt branch November 5, 2024 14:31
@marsella marsella linked an issue Nov 5, 2024 that may be closed by this pull request
10 tasks
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.

Bring NTT sections of ML-KEM up to gold standard
2 participants