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

Add Fast NTT and Inverse NTT for ML-KEM #95

Merged
merged 10 commits into from
Jul 24, 2024

Conversation

rod-chapman
Copy link
Contributor

This PR adds "fast" O(N log N) NTT and Inverse NTT functions for ML-KEM.

Note

  1. I moved the "coerceSize" function from Common:ntt to Common::utils so it could be re-used in ML-KEM
  2. With the new code, KATs pass OK.
  3. Time to run ONE KAT with the old code (on am M1 MacBook Pro) is 38s. With new code, it's 22.5s.
  4. A single run of NTTInv(NTT f) with the old code takes 5s. With new code it's 140ms - about 36 times faster.

…ML-KEM

Signed-off-by: Rod Chapman <rodchap@amazon.com>
since it is a constant for all ML-KEM variants.

Moving its declaration into specification.cry also makes
it a static value for the type constraint solver.

Signed-off-by: Rod Chapman <rodchap@amazon.com>
Also adds properties and proofs to show that:
 NaiveNTT and NaiveInvNTT are inverting
 fast_ntt and fast_invntt are inverting
 NaiveNTT is equivalent to fast_ntt
 NaiveInvNTT is equivalent to fast_invntt

Signed-off-by: Rod Chapman <rodchap@amazon.com>
@rod-chapman rod-chapman changed the title Add Fast NTT and Inverse NTT Add Fast NTT and Inverse NTT for ML-KEM Jul 18, 2024
…mments.

Signed-off-by: Rod Chapman <rodchap@amazon.com>
Copy link
Contributor

@marsella marsella 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 these changes! Those are great speedups. I have a few thoughts & discussion points; the only blocking comment is adding a reference to the fast NTT algorithm.

It also looked like we needed to specifically approve CI running from external forks, I just set that to run now. We recently added a job that requires copyright notices to be added to files, which is going to fail because we historically have not added those consistently. I added some comments throughout with the correct notices, please add your email if you wish and paste those in to fix CI.

Comment on lines +294 to +310
//////////////////////////////////////////////////////////////
// NTT "dispatcher"
//
// Here, we can choose to call either the naive or fast NTT
//////////////////////////////////////////////////////////////

NTT' : Z_q_256 -> Z_q_256
NTT' f = ParametricNTT f zeta
// fast
NTT' f = fast_ntt f
// slow
//NTT' f = NaiveNTT f

NTTInv' : Z_q_256 -> Z_q_256
NTTInv' f = [term*(recip 128) | term <- ParametricNTTInv f (recip zeta)]

CorrectnessNTT : Z_q_256 -> Bit
property CorrectnessNTT f = NTTInv' (NTT' f) == f

// fast
NTTInv' f = fast_invntt f
// slow
//NTTInv' f = NaiveNTTInv f
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: We've discussed internally moving away from in-code switches like this and replacing them with interfaces (but only for the case of changing parameter sizes like keys). We haven't really discussed how to approach alternative implementations of a single algorithm. I'm debating between two options:

  • We could parameterize the whole file by the NTT version (fast vs slow) so it's more obvious from a user perspective (a) that there's a choice and (b) that you can choose the fast version
  • We might just fix the fast version and remove the commented-out switch here. Since they're proven equivalent, I'm not sure there's any good reason for someone to choose the slow version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would be happy to change that in the future to whatever coding style and policy you settle on. Can we defer that decision to another day?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, let's defer. This is not necessary for this PR, just wanted to flag it as a discussion point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me.

Primitive/Asymmetric/Cipher/ML-KEM/ml_kem1024.cry Outdated Show resolved Hide resolved
Common/utils.cry Outdated Show resolved Hide resolved
Common/ntt.cry Outdated Show resolved Hide resolved
Signed-off-by: Rod Chapman <rodchap@amazon.com>
…ated Issue

Signed-off-by: Rod Chapman <rodchap@amazon.com>
rod-chapman and others added 4 commits July 24, 2024 13:03
Signed-off-by: Rod Chapman <rodchap@amazon.com>
Signed-off-by: Rod Chapman <rodchap@amazon.com>
Not all of our files were committed by people at Galois.
Copy link
Contributor

@marsella marsella left a comment

Choose a reason for hiding this comment

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

Thanks for iterating on these changes!

@rod-chapman
Copy link
Contributor Author

Super, and thanks to everyone for their efforts and help. Can someone press the "Merge" button now??

@marsella marsella merged commit 42615d5 into GaloisInc:master Jul 24, 2024
2 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.

4 participants