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

More kyber code refactoring #135

Merged
merged 24 commits into from
Nov 25, 2023
Merged

More kyber code refactoring #135

merged 24 commits into from
Nov 25, 2023

Conversation

xvzcf
Copy link
Contributor

@xvzcf xvzcf commented Nov 22, 2023

This PR is a grab-bag of various changes to Kyber to make things clearer, fix constant time issues and such:

  • ntt.rs had code for both the NTT and functions that used the NTT in matrix/vector multiplication. These latter functions have been moved to a matrix.rs file.
  • I renamed KyberPolynomialRingElement and KyberFieldElement to just PolynomialRingElement and FieldElement (they're in the Kyber code, so the Kyber prefix is uncessary)
  • Some field elements are in the montgomery domain as they've been multiplied by R^{-1}, others have been multiplied by R, so multiplications take elements from the montgomery domain to the standard domain, some the other way around, so I added some type aliases and corresponding functions to try and document this. These type aliases could become tuple structs later
  • I renamed the cbd function in ind_cpa.rs to sample_vector_cbd_then_ntt and expanded its use.
  • I added two new constant-time functions for compressing and decompressing the coefficients of the message ring element, since the message is secret information
  • The at_layer macros used in the NTT have been converted to functions that get inlined

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.

Please describe what this PR is doing.

src/kem/kyber/arithmetic.rs Outdated Show resolved Hide resolved
src/kem/kyber/arithmetic.rs Show resolved Hide resolved
src/kem/kyber/arithmetic.rs Outdated Show resolved Hide resolved
src/kem/kyber/arithmetic.rs Outdated Show resolved Hide resolved
src/kem/kyber/matrix.rs Outdated Show resolved Hide resolved
src/kem/kyber/matrix.rs Outdated Show resolved Hide resolved
src/kem/kyber/ntt.rs Outdated Show resolved Hide resolved
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.

Thanks. Generally looks like good cleanup.
Let's add comments to things you're touching to get comments back in here.

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.

Thanks, only one nit.
Let's make sure to get laxing on CI before merging this. But otherwise this is good to go.

src/kem/kyber/ntt.rs Outdated Show resolved Hide resolved
@xvzcf xvzcf force-pushed the goutam/kyber-improvements branch 7 times, most recently from e6e3ef6 to ea870ce Compare November 24, 2023 19:55
Copy link
Contributor

@karthikbhargavan karthikbhargavan left a comment

Choose a reason for hiding this comment

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

All looks good, thanks.

src/kem/kyber/arithmetic.rs Show resolved Hide resolved
src/kem/kyber/serialize.rs Show resolved Hide resolved
src/kem/kyber/arithmetic.rs Show resolved Hide resolved
@franziskuskiefer franziskuskiefer merged commit 3acfa1d into dev Nov 25, 2023
14 checks passed
@franziskuskiefer franziskuskiefer deleted the goutam/kyber-improvements branch November 25, 2023 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants