feat: negacyclic ntt - #28
Conversation
📝 WalkthroughWalkthroughAdds negacyclic and cyclic NTT implementations plus a generic NttContext, small-modulus field element types/configs for Kyber and Dilithium, extends FieldConfig with PRIMITIVE_2NTH_ROOT and NTT_DEGREE, and adds tests and THEORY.md documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant NttContext
participant PolyA
participant PolyB
participant Pointwise
participant Result
Caller->>NttContext: mul(a, b)
Note over NttContext,PolyA: Forward transform (pre-scale by ψ)
NttContext->>PolyA: ntt_negacyclic(a)
PolyA-->>NttContext: a_hat
Note over NttContext,PolyB: Forward transform (pre-scale by ψ)
NttContext->>PolyB: ntt_negacyclic(b)
PolyB-->>NttContext: b_hat
rect rgb(240,250,240)
Note over NttContext,Pointwise: Pointwise multiplication
NttContext->>Pointwise: a_hat[i] * b_hat[i]
Pointwise-->>NttContext: c_hat
end
Note over NttContext,Result: Inverse transform (Gentleman‑Sande, post-scale by ψ⁻¹, scale by n⁻¹)
NttContext->>Result: intt_negacyclic(c_hat)
Result-->>Caller: product polynomial
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
src/poly/ntt/mod.rs (1)
205-228: Consider accepting immutable references formul().The
mulmethod mutates both input slicesaandbin-place (via NTT transformation), which may be surprising to users who expect their original polynomials to remain unchanged after multiplication. The signature&mut [FieldElement<C>]signals mutation, but the docstring doesn't warn about this destructive behavior.Consider either:
- Documenting that inputs are destroyed, or
- Accepting
&[FieldElement<C>]and cloning internally if preserving inputs is preferred🔎 Suggested documentation addition
/// Polynomial multiplication in Rq = Zq[X]/(X^N+1). /// + /// # Note + /// This method transforms `a` and `b` in-place to NTT domain. + /// The original coefficient values are destroyed. + /// /// # Panics /// Panics if `a.len() != self.n` or `b.len() != self.n`.tests/field_element_test.rs (1)
161-171: Test comment may be misleading.The comment on line 164 says "Test Clone" but the code now uses
let b = awhich tests Copy semantics (sinceFieldElementimplementsCopy). This is functionally the same for types implementing both traits, but the test no longer explicitly exercises.clone().Consider either updating the comment or keeping
.clone()to explicitly test the Clone implementation:🔎 Option 1: Update comment
- // Test Clone + // Test Copy (FieldElement implements Copy) let b = a;src/lib.rs (1)
30-36: Redundant re-exports on lines 32-36.Line 30 already uses
pub use crate::poly::ntt::*, which exposesNttContext,intt_negacyclic,mul_negacyclic,ntt_negacyclic,DilithiumFieldConfig, andKyberFieldConfigfrom the ntt module.The explicit re-exports on lines 32-36 are redundant. They do serve as API documentation for crate users, so you may keep them for clarity or remove them to reduce duplication—this is a minor stylistic choice.
src/poly/ntt/negacyclic.rs (1)
141-155: Consider non-destructive API for polynomial multiplication.The current implementation mutates both input slices
aandb, transforming them in-place. While documented, this is potentially surprising for a multiplication operation where users typically expect inputs to remain unchanged.Consider either:
- Taking
&[FieldElement<C>]and cloning internally, or- Renaming to emphasize the mutation (e.g.,
mul_negacyclic_inplace)The current approach does avoid extra allocations if the caller doesn't need the original polynomials.
tests/negacyclic_ntt_test.rs (1)
44-64: Multiplication test could verify expected coefficients.The test currently only checks that the result length is correct. Consider adding assertions for the expected coefficients of (1+x)² in the negacyclic ring Zq[X]/(X^N+1).
For N=8: (1+x)² = 1 + 2x + x², so you could verify:
assert_eq!(result[0].to_u1024().0[0], 1); assert_eq!(result[1].to_u1024().0[0], 2); assert_eq!(result[2].to_u1024().0[0], 1); for i in 3..8 { assert!(result[i].is_zero()); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/field/config.rssrc/lib.rssrc/poly/ntt/config.rssrc/poly/ntt/cyclic.rssrc/poly/ntt/mod.rssrc/poly/ntt/negacyclic.rstests/field_element_test.rstests/integration_test.rstests/negacyclic_ntt_test.rstests/property_tests.rstests/u1024_test.rs
🧰 Additional context used
🧬 Code graph analysis (4)
src/lib.rs (3)
src/poly/ntt/cyclic.rs (1)
ntt(22-55)src/poly/ntt/mod.rs (1)
ntt(111-152)src/poly/ntt/negacyclic.rs (6)
intt_negacyclic(82-126)intt_negacyclic(170-170)mul_negacyclic(141-155)ntt_negacyclic(31-73)ntt_negacyclic(169-169)ntt_negacyclic(193-193)
src/poly/ntt/negacyclic.rs (3)
src/poly/ntt/cyclic.rs (1)
bit_reverse(9-19)src/poly/ntt/mod.rs (1)
new(75-105)src/field/element.rs (1)
to_u1024(146-148)
tests/negacyclic_ntt_test.rs (4)
src/poly/ntt/cyclic.rs (3)
ntt(22-55)bit_reverse(9-19)intt(58-72)src/poly/ntt/mod.rs (3)
ntt(111-152)intt(158-199)new(75-105)src/poly/ntt/negacyclic.rs (6)
intt_negacyclic(82-126)intt_negacyclic(170-170)mul_negacyclic(141-155)ntt_negacyclic(31-73)ntt_negacyclic(169-169)ntt_negacyclic(193-193)src/field/element.rs (1)
to_u1024(146-148)
src/poly/ntt/mod.rs (3)
src/poly/ntt/cyclic.rs (3)
bit_reverse(9-19)intt(58-72)ntt(22-55)src/poly/ntt/negacyclic.rs (5)
intt_negacyclic(82-126)intt_negacyclic(170-170)ntt_negacyclic(31-73)ntt_negacyclic(169-169)ntt_negacyclic(193-193)src/field/element.rs (1)
to_u1024(146-148)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test and Coverage
🔇 Additional comments (17)
src/poly/ntt/cyclic.rs (1)
1-5: LGTM!The module documentation clearly describes the purpose of cyclic NTT and differentiates it from the negacyclic variant being added in this PR.
src/poly/ntt/mod.rs (3)
1-36: LGTM!The module documentation is comprehensive with clear usage examples for both cyclic and negacyclic NTT. The re-exports are well-organized for backward compatibility.
57-68: LGTM!The
NttContextstruct design is clean. Precomputingpsi_powers,psi_inv_powers, andn_invis efficient for repeated NTT operations.
231-267: LGTM!The tests provide good coverage for the
NttContextincluding roundtrip correctness, zero element handling, and panic behavior for invalid sizes.tests/u1024_test.rs (1)
69-72: LGTM!The assertion style changes from negated comparisons (
!(b < a)) to direct relational operators (b >= a) improve readability without changing the test logic.tests/property_tests.rs (1)
36-37: LGTM!The change from borrowed comparison (
&res_oracle >= &modulus) to owned comparison (res_oracle >= modulus) is cleaner and semantically equivalent forBigUint.tests/integration_test.rs (2)
49-49: LGTM!Using
assert!(!carry)is more idiomatic thanassert_eq!(carry, false)for boolean assertions.
245-246: LGTM!The assertion style change from negated comparisons to direct relational operators is consistent with similar changes in other test files.
tests/field_element_test.rs (1)
179-179: LGTM!Using
!debug_str.is_empty()is more idiomatic than checkinglen() > 0.src/field/config.rs (1)
28-35: LGTM!The new associated constants properly extend
FieldConfigwith backward-compatible defaults:
PRIMITIVE_2NTH_ROOTdefaults toROOT_OF_UNITYfor existing configsNTT_DEGREEdefaults to 256, matching Kyber/Dilithium parametersKyber and Dilithium field configurations correctly override
PRIMITIVE_2NTH_ROOTwith properly computed values (17 for Kyber, 1753 for Dilithium) and include mathematical verification in the test documentation that these satisfy the required properties (ζ^256 ≡ 1 mod 3329 for Kyber, ψ^256 ≡ -1 mod 8380417 for Dilithium).src/poly/ntt/negacyclic.rs (1)
1-22: Documentation is clear and well-structured.The mathematical background section effectively explains the negacyclic NTT transform and its application to lattice-based cryptography.
tests/negacyclic_ntt_test.rs (5)
1-12: Imports and setup are well-organized.The type alias
FEforFieldElement<DefaultFieldConfig>improves readability throughout the test file.
70-111: Standalone function tests effectively validate core properties.The tests cover essential NTT properties:
- Round-trip correctness (invertibility)
- Multiplicative identity
- Zero preservation
117-141: Backward compatibility tests ensure existing functionality is preserved.Testing that cyclic NTT and bit_reverse still work correctly after adding negacyclic NTT prevents regressions.
147-171: Linearity test correctly validates the NTT's linear transformation property.This test effectively demonstrates that the NTT preserves addition, a fundamental requirement for any correct NTT implementation.
1-171: Comprehensive and well-structured test suite.The test organization is excellent with clear sections for:
- NttContext API tests
- Standalone function tests
- Backward compatibility
- Mathematical properties (linearity)
The tests cover critical correctness properties and edge cases. Once the twiddle factor bug in the implementation is fixed, this test suite should provide good confidence in the negacyclic NTT implementation.
src/poly/ntt/config.rs (1)
8-52: The configuration is correct per the Kyber specification. Kyber intentionally uses an "incomplete" or adapted NTT design (not a standard negacyclic NTT) for the polynomial ring modulo x^256 + 1. The key property is that ζ = 17 satisfies 17^128 ≡ −1 (mod 3329), which enables the modified NTT that factors x^256 + 1 into 128 quadratic factors. This is the intended behavior, not a violation of negacyclic NTT requirements. Setting bothROOT_OF_UNITYandPRIMITIVE_2NTH_ROOTto 17 is correct.Likely an incorrect or invalid review comment.
| /// Dilithium field configuration: q = 8,380,417, n = 256. | ||
| /// | ||
| /// Dilithium uses a complete NTT with a 512th primitive root of unity. | ||
| /// r = 1753 satisfies r^256 ≡ -1 (mod q). | ||
| #[derive(Copy, Clone, Debug, Default, PartialEq, Eq)] | ||
| pub struct DilithiumFieldConfig; | ||
|
|
||
| impl FieldConfig for DilithiumFieldConfig { | ||
| // q = 8380417 | ||
| const MODULUS: U1024 = U1024([8380417, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]); | ||
| const MODULUS_BITS: u32 = 23; | ||
|
|
||
| // R^2 mod 8380417 where R = 2^1024 | ||
| // Computed: (2^1024 mod 8380417)^2 mod 8380417 = 4628081 | ||
| const R2: U1024 = U1024([4628081, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]); | ||
|
|
||
| // N' such that N * N' ≡ -1 (mod 2^64) where N = 8380417 | ||
| // Computed: 16714476285912408063 | ||
| const N_PRIME: U1024 = U1024([ | ||
| 16714476285912408063, | ||
| 0, | ||
| 0, | ||
| 0, | ||
| 0, | ||
| 0, | ||
| 0, | ||
| 0, | ||
| 0, | ||
| 0, | ||
| 0, | ||
| 0, | ||
| 0, | ||
| 0, | ||
| 0, | ||
| 0, | ||
| ]); | ||
|
|
||
| /// r = 1753 is a primitive 512th root of unity mod 8380417. | ||
| /// This means r^512 ≡ 1 and r^256 ≡ -1. | ||
| const ROOT_OF_UNITY: U1024 = U1024([1753, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]); | ||
|
|
||
| /// ψ = 1753, satisfying ψ^256 ≡ -1 (mod q). | ||
| const PRIMITIVE_2NTH_ROOT: U1024 = U1024([1753, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]); | ||
|
|
||
| const NTT_DEGREE: usize = 256; | ||
| } |
There was a problem hiding this comment.
ROOT_OF_UNITY incorrectly set for DilithiumFieldConfig.
Lines 91-93 correctly identify r = 1753 as a primitive 512th root of unity (ψ) satisfying ψ^512 = 1 and ψ^256 = -1. This is the correct PRIMITIVE_2NTH_ROOT for negacyclic NTT.
However, line 93 also sets ROOT_OF_UNITY = 1753, which is incorrect. For cyclic NTT, ROOT_OF_UNITY should be a primitive 256th root of unity (ω), which equals ψ² = 1753² mod 8380417 = 3072169.
Impact:
- If cyclic NTT (
nttfunction from cyclic.rs) is called withDilithiumFieldConfig, it will use the wrong root and produce incorrect results - If only negacyclic NTT is intended for Dilithium, the incorrect value is misleading and could cause future bugs
🔎 Proposed fix
- /// r = 1753 is a primitive 512th root of unity mod 8380417.
- /// This means r^512 ≡ 1 and r^256 ≡ -1.
- const ROOT_OF_UNITY: U1024 = U1024([1753, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]);
+ /// ω = ψ^2 = 1753^2 mod 8380417 = 3072169 is a primitive 256th root of unity.
+ const ROOT_OF_UNITY: U1024 = U1024([3072169, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]);
- /// ψ = 1753, satisfying ψ^256 ≡ -1 (mod q).
+ /// ψ = 1753 is a primitive 512th root, satisfying ψ^512 ≡ 1 and ψ^256 ≡ -1 (mod q).
const PRIMITIVE_2NTH_ROOT: U1024 = U1024([1753, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Dilithium field configuration: q = 8,380,417, n = 256. | |
| /// | |
| /// Dilithium uses a complete NTT with a 512th primitive root of unity. | |
| /// r = 1753 satisfies r^256 ≡ -1 (mod q). | |
| #[derive(Copy, Clone, Debug, Default, PartialEq, Eq)] | |
| pub struct DilithiumFieldConfig; | |
| impl FieldConfig for DilithiumFieldConfig { | |
| // q = 8380417 | |
| const MODULUS: U1024 = U1024([8380417, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]); | |
| const MODULUS_BITS: u32 = 23; | |
| // R^2 mod 8380417 where R = 2^1024 | |
| // Computed: (2^1024 mod 8380417)^2 mod 8380417 = 4628081 | |
| const R2: U1024 = U1024([4628081, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]); | |
| // N' such that N * N' ≡ -1 (mod 2^64) where N = 8380417 | |
| // Computed: 16714476285912408063 | |
| const N_PRIME: U1024 = U1024([ | |
| 16714476285912408063, | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| ]); | |
| /// r = 1753 is a primitive 512th root of unity mod 8380417. | |
| /// This means r^512 ≡ 1 and r^256 ≡ -1. | |
| const ROOT_OF_UNITY: U1024 = U1024([1753, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]); | |
| /// ψ = 1753, satisfying ψ^256 ≡ -1 (mod q). | |
| const PRIMITIVE_2NTH_ROOT: U1024 = U1024([1753, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]); | |
| const NTT_DEGREE: usize = 256; | |
| } | |
| /// Dilithium field configuration: q = 8,380,417, n = 256. | |
| /// | |
| /// Dilithium uses a complete NTT with a 512th primitive root of unity. | |
| /// r = 1753 satisfies r^256 ≡ -1 (mod q). | |
| #[derive(Copy, Clone, Debug, Default, PartialEq, Eq)] | |
| pub struct DilithiumFieldConfig; | |
| impl FieldConfig for DilithiumFieldConfig { | |
| // q = 8380417 | |
| const MODULUS: U1024 = U1024([8380417, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]); | |
| const MODULUS_BITS: u32 = 23; | |
| // R^2 mod 8380417 where R = 2^1024 | |
| // Computed: (2^1024 mod 8380417)^2 mod 8380417 = 4628081 | |
| const R2: U1024 = U1024([4628081, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]); | |
| // N' such that N * N' ≡ -1 (mod 2^64) where N = 8380417 | |
| // Computed: 16714476285912408063 | |
| const N_PRIME: U1024 = U1024([ | |
| 16714476285912408063, | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| ]); | |
| /// ω = ψ^2 = 1753^2 mod 8380417 = 3072169 is a primitive 256th root of unity. | |
| const ROOT_OF_UNITY: U1024 = U1024([3072169, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]); | |
| /// ψ = 1753 is a primitive 512th root, satisfying ψ^512 ≡ 1 and ψ^256 ≡ -1 (mod q). | |
| const PRIMITIVE_2NTH_ROOT: U1024 = U1024([1753, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]); | |
| const NTT_DEGREE: usize = 256; | |
| } |
🤖 Prompt for AI Agents
In src/poly/ntt/config.rs around lines 54 to 99, the DilithiumFieldConfig
currently sets ROOT_OF_UNITY to 1753 which is the primitive 512th root (ψ) not
the primitive 256th root (ω); change ROOT_OF_UNITY to ψ² mod 8380417 = 3072169
(i.e., replace the U1024 value for ROOT_OF_UNITY with 3072169) so cyclic NTT
uses the correct 256th root of unity while leaving PRIMITIVE_2NTH_ROOT as 1753.
| let log_len = len.trailing_zeros(); | ||
| let factor = 32 - log_len; | ||
|
|
||
| // ω = ψ^2, then raised to appropriate power for this layer | ||
| let omega_base = FieldElement::<C>::new(C::PRIMITIVE_2NTH_ROOT); | ||
| let mut w_len = omega_base * omega_base; | ||
| for _ in 0..factor { | ||
| w_len = w_len * w_len; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for NTT_DEGREE and related constant definitions
rg -n 'NTT_DEGREE|NTT_LEN|MAX.*NTT|log_len' --type rust -i | head -40Repository: Tranduy1dol/lumen-math
Length of output: 924
🏁 Script executed:
#!/bin/bash
# Also search for any documentation or comments about degree limits
rg -n -B2 -A2 'trailing_zeros|factor.*32|log_len' --type rust src/poly/ntt/Repository: Tranduy1dol/lumen-math
Length of output: 1891
🏁 Script executed:
#!/bin/bash
# Check the full context of the NTT module to understand degree constraints
fd '.*ntt.*' --type f | xargs -I {} sh -c 'echo "=== {} ===" && wc -l {} | head -1'Repository: Tranduy1dol/lumen-math
Length of output: 258
🏁 Script executed:
#!/bin/bash
# Get the full context of the loop containing the factor calculation (lines 120-145)
sed -n '120,145p' src/poly/ntt/mod.rsRepository: Tranduy1dol/lumen-math
Length of output: 948
🏁 Script executed:
#!/bin/bash
# Search for how the functions using this pattern are called
rg -n 'forward_transform|inverse_transform' --type rust -B2 -A5 src/poly/ntt/mod.rs | head -50Repository: Tranduy1dol/lumen-math
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check if there are any tests that specify the maximum degree supported
rg -n 'degree|NTT_DEGREE' --type rust tests/ | head -20Repository: Tranduy1dol/lumen-math
Length of output: 1668
The hardcoded 32 in factor = 32 - log_len is currently safe but lacks defensive documentation.
Since all NTT_DEGREE definitions in the codebase are hardcoded to 256 (2^8), log_len never exceeds 8, so factor stays in the safe range. However, the constant lacks explanation and would silently produce incorrect results if someone attempted to extend the code to support NTT degrees beyond 2^32. Consider adding a comment explaining that this calculation depends on NTT_DEGREE fitting within 32 bits, or guard against future misuse with an assertion.
🤖 Prompt for AI Agents
In src/poly/ntt/mod.rs around lines 130 to 138, the expression `factor = 32 -
log_len` uses a hardcoded 32 without explanation and can underflow if `log_len >
32`; add a short comment stating this code assumes NTT_DEGREE <= 2^32 (current
code uses 256 = 2^8), and add a defensive assertion such as `assert!(log_len <=
32, "NTT length log exceeds 32 bits; update factor calculation")` before
computing factor so future increases fail fast and guide the maintainer to
update the logic.
- Add small.rs with KyberFieldElement (u16) and DilithiumFieldElement (u32) using Barrett reduction for correct small-modulus arithmetic - Add deprecation warnings to U1024-based KyberFieldConfig/DilithiumFieldConfig - Fix ROOT_OF_UNITY for Dilithium: use ω = 3073009 (ψ² mod q) - Fix twiddle factor calculation: replace hardcoded 32 with n.trailing_zeros() - Add THEORY.md documenting mathematical concepts - Update issues.md with memory optimization issues New exports: - KyberFieldElement, DilithiumFieldElement - KYBER_Q, KYBER_ZETA, DILITHIUM_Q, DILITHIUM_PSI, DILITHIUM_OMEGA All 53 tests pass.
There was a problem hiding this comment.
Actionable comments posted: 3
Fix all issues with AI Agents 🤖
In @src/poly/ntt/config.rs:
- Around line 195-198: The comment documenting Dilithium's ω is incorrect: it
states `ω = 3072169` but the actual constant is `ROOT_OF_UNITY = 3073009`;
update the comment to `ω = 3073009` (or replace the literal with a reference to
ROOT_OF_UNITY) so the documentation matches the code (look for the comment block
mentioning Kyber/Dilithium and the constant ROOT_OF_UNITY in
src/poly/ntt/config.rs).
In @src/poly/ntt/mod.rs:
- Around line 221-244: The mul function currently mutates its inputs a and b by
calling self.ntt(a) and self.ntt(b) and never restores them; either document
this side effect in the mul docstring or make mul non-mutating by changing the
signature to take &[FieldElement<C>] and cloning into local Vec<FieldElement<C>>
(perform self.ntt and multiplication on the clones, then self.intt on the
result), or alternatively restore inputs by calling self.intt(a) and
self.intt(b) before returning; update references to mul, ntt, intt and the
parameters a/b accordingly so callers won’t get unexpected mutated arrays.
In @src/poly/ntt/small.rs:
- Around line 266-270: The doc comment on the omega() const fn is incorrect: it
states "ω = 3072169" but the actual constant DILITHIUM_OMEGA (used in
Self(DILITHIUM_OMEGA)) is 3073009; update the documentation comment above pub
const fn omega() -> Self to state the correct value "ω = 3073009" so the comment
matches the DILITHIUM_OMEGA symbol.
🧹 Nitpick comments (5)
src/poly/ntt/cyclic.rs (1)
32-36: Good fix: dynamic twiddle factor calculation.The change from a hardcoded
32to computinglog_ndynamically is correct and more robust. However,log_nis invariant across loop iterations—consider hoisting it before thewhileloop.🔎 Optional: hoist invariant computation
bit_reverse(coeffs); + let log_n = (n as u32).trailing_zeros(); let mut len = 2; while len <= n { let half_len = len / 2; // Compute twiddle factor ω^(n/len) for this layer. // Starting from ROOT_OF_UNITY (Nth root), we square it (log2(n) - log2(len)) times. - let log_n = (n as u32).trailing_zeros(); let log_len = len.trailing_zeros(); let factor = log_n - log_len;THEORY.md (1)
27-30: Add language specifiers to fenced code blocks.Per markdownlint MD040, fenced code blocks should have a language specified for syntax highlighting and accessibility. Consider adding
textorpseudocodefor algorithm descriptions.Also applies to: 81-87, 113-117, 125-129, 148-150, 155-157, 163-174, 223-237, 248-253, 257-261
src/poly/ntt/mod.rs (1)
141-152:log_nandomega_baseare loop-invariant.Similar to the cyclic NTT,
log_nandomega_baseare recomputed every iteration but don't change. Consider hoisting them before the loops for a minor optimization.Also applies to: 184-194
src/poly/ntt/small.rs (2)
44-52:# Safetyheader is misleading for non-unsafefunctions.The
from_raw()methods usedebug_assert!which only panics in debug builds. The# Safetydoc header typically indicates anunsafe fnwith invariants the caller must uphold. Consider:
- Changing to
# Panics(if you want debug panics documented)- Or removing the section and just documenting the precondition requirement
🔎 Suggested documentation
/// Creates a field element from a raw value (must be < q). /// - /// # Safety - /// The caller must ensure `value < KYBER_Q`. + /// # Preconditions + /// `value` must be less than `KYBER_Q`. In debug builds, this is + /// checked with a `debug_assert!`. #[inline] pub const fn from_raw(value: u16) -> Self {Also applies to: 232-240
278-290: Barrett reduction useswhileloop instead of single conditional.For Dilithium, the Barrett reduction uses a
whileloop (lines 286-288). Given the reduction constant choice, at most one or two subtractions should be needed. A bounded check (singleifor at most twoifs) may be clearer and avoid any theoretical unbounded loop concern in edge cases.🔎 Suggested change
fn barrett_reduce(x: u64) -> u32 { let t = ((x as u128 * DILITHIUM_BARRETT as u128) >> 48) as u64; let mut r = x - t * (DILITHIUM_Q as u64); - while r >= DILITHIUM_Q as u64 { + // At most 2 reductions needed given Barrett constant precision + if r >= DILITHIUM_Q as u64 { + r -= DILITHIUM_Q as u64; + } + if r >= DILITHIUM_Q as u64 { r -= DILITHIUM_Q as u64; } r as u32 }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
THEORY.mdsrc/lib.rssrc/poly/ntt/config.rssrc/poly/ntt/cyclic.rssrc/poly/ntt/mod.rssrc/poly/ntt/negacyclic.rssrc/poly/ntt/small.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/poly/ntt/negacyclic.rs
- src/lib.rs
🧰 Additional context used
🧬 Code graph analysis (3)
src/poly/ntt/config.rs (3)
src/poly/ntt/cyclic.rs (1)
ntt(22-58)src/poly/ntt/mod.rs (2)
ntt(122-166)new(86-116)src/poly/ntt/small.rs (11)
zeta(74-76)value(56-58)value(244-246)psi(262-264)omega(268-270)new(40-42)new(222-224)one(68-70)one(256-258)to_u1024(158-160)to_u1024(352-354)
src/poly/ntt/mod.rs (3)
src/poly/ntt/cyclic.rs (3)
bit_reverse(9-19)intt(61-75)ntt(22-58)src/poly/ntt/negacyclic.rs (5)
intt_negacyclic(85-132)intt_negacyclic(176-176)ntt_negacyclic(31-76)ntt_negacyclic(175-175)ntt_negacyclic(199-199)src/poly/ntt/small.rs (13)
new(40-42)new(222-224)psi(262-264)from_u64(228-230)one(68-70)one(256-258)omega(268-270)mul(183-185)mul(377-379)zero(62-64)zero(250-252)is_zero(80-82)is_zero(274-276)
src/poly/ntt/small.rs (2)
src/poly/ntt/mod.rs (2)
new(86-116)mul(221-244)src/poly/univariate.rs (1)
x(51-55)
🪛 markdownlint-cli2 (0.18.1)
THEORY.md
27-27: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
81-81: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
113-113: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
125-125: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
148-148: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
155-155: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
163-163: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
221-221: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
223-223: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
248-248: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
257-257: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test and Coverage
🔇 Additional comments (10)
src/poly/ntt/cyclic.rs (1)
1-5: LGTM!Clear module-level documentation explaining the cyclic NTT context.
src/poly/ntt/config.rs (3)
1-36: Good deprecation strategy with clear migration path.The deprecation warnings, documentation, and guidance to use
small::KyberFieldElementare well-structured. The explanation of U1024 Montgomery incompatibility with small moduli is helpful for users.
126-135: ROOT_OF_UNITY corrected to ω = ψ².The previous review concern has been addressed—
ROOT_OF_UNITYis now3073009(ψ² mod q), distinct fromPRIMITIVE_2NTH_ROOTwhich remains1753(ψ).
137-233: Test coverage is well-organized.The separation between working small-field tests and ignored legacy U1024 tests is appropriate given the documented Montgomery incompatibility.
src/poly/ntt/mod.rs (3)
1-49: Well-documented module with clear API guidance.The module documentation clearly distinguishes between small-modulus types (recommended for production) and generic NTT (for large moduli), with helpful code examples.
68-116: Clean NttContext design with appropriate precomputation.The constructor properly validates power-of-two requirements and precomputes ψ powers, ψ⁻¹ powers, and n⁻¹ for efficient reuse.
247-284: Good test coverage for NttContext.The tests cover round-trip correctness, zero-input behavior, and invalid size handling.
src/poly/ntt/small.rs (3)
1-16: Clear module documentation with usage examples.The documentation effectively explains the purpose and usage of small-modulus field elements with Barrett reduction.
84-96: Kyber Barrett reduction is correctly bounded.The single
ifcorrection is appropriate for the 32-bit Barrett reduction with q=3329.
394-500: Comprehensive test coverage for both field element types.Tests verify basic arithmetic, primitive root properties (ζ^256=1, ψ^256=-1, ω^256=1), and inverse operations. The
test_dilithium_psi_squared_is_omegatest is particularly valuable for validating the root relationship.
- Bump version to 1.3.0 - Add Negacyclic NTT and Small Field Types - Fix NTT twiddle factor calculation - Update README.md and CHANGELOG.md - Add THEORY.md documentation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
README.md (1)
147-151: Minor: Inconsistent list indentation flagged by linter.The nested list items use 4-space indentation, but markdownlint expects 2-space indentation for sub-items. This is a stylistic preference and renders correctly on GitHub, but you may want to adjust for linter consistency.
🔎 Proposed fix
- **`poly`**: Polynomial arithmetic. - `dense`: Dense polynomial representation and operations. - - `ntt`: Number Theoretic Transform (cyclic and negacyclic) implementations. - - `mod.rs`: Generic `NttContext` and re-exports. - - `cyclic.rs`: Standard cyclic NTT over $Z_q[X]/(X^N-1)$. - - `negacyclic.rs`: Negacyclic NTT over $Z_q[X]/(X^N+1)$. - - `small.rs`: Specialized small-modulus field types. + - `ntt`: Number Theoretic Transform (cyclic and negacyclic) implementations. + - `mod.rs`: Generic `NttContext` and re-exports. + - `cyclic.rs`: Standard cyclic NTT over $Z_q[X]/(X^N-1)$. + - `negacyclic.rs`: Negacyclic NTT over $Z_q[X]/(X^N+1)$. + - `small.rs`: Specialized small-modulus field types.THEORY.md (2)
134-138: Minor: Add language specifier to pseudocode block.The linter flags this fenced code block as missing a language specifier. Consider adding
textorplaintextto improve syntax highlighting behavior.🔎 Proposed fix
-``` +```text result = c_n for i from n-1 down to 0: result = result · x + c_i</details> --- `194-205`: **Minor: Add language specifier to Cooley-Tukey pseudocode block.** Same issue as above—add `text` or `plaintext` for consistency. <details> <summary>🔎 Proposed fix</summary> ```diff -``` +```text for each layer len = 2, 4, 8, ..., N: ω_len = ω^(N/len) // twiddle factor for each group: w = 1 for j in half_len: u = coeffs[j] v = coeffs[j + half_len] · w coeffs[j] = u + v coeffs[j + half_len] = u - v w = w · ω_len</details> </blockquote></details> <details> <summary>src/poly/ntt/small.rs (1)</summary><blockquote> `278-290`: **Consider replacing `while` loop with single correction.** For the Dilithium Barrett reduction, the approximation error is bounded such that `r` can be at most `2q - 1` after the initial subtraction. This means only one correction is ever needed, making the `while` loop equivalent to an `if`. Using an `if` instead of `while` would be marginally faster and make the bound explicit, matching the Kyber implementation pattern. <details> <summary>🔎 Proposed fix</summary> ```diff fn barrett_reduce(x: u64) -> u32 { // t = floor(x * BARRETT / 2^48) ≈ floor(x / q) let t = ((x as u128 * DILITHIUM_BARRETT as u128) >> 48) as u64; // r = x - t * q let mut r = x - t * (DILITHIUM_Q as u64); - // Final correction (r might be >= q) - while r >= DILITHIUM_Q as u64 { + // Final correction (r might be >= q, but at most 2q-1) + if r >= DILITHIUM_Q as u64 { r -= DILITHIUM_Q as u64; } r as u32 }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.mdCargo.tomlREADME.mdTHEORY.mdsrc/poly/ntt/config.rssrc/poly/ntt/cyclic.rssrc/poly/ntt/mod.rssrc/poly/ntt/small.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/poly/ntt/cyclic.rs
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
147-147: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
148-148: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
149-149: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
150-150: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
151-151: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
THEORY.md
134-134: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
194-194: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test and Coverage
🔇 Additional comments (12)
Cargo.toml (1)
3-3: LGTM!Version bump to 1.3.0 is appropriate for the new features (negacyclic NTT, small-modulus field types) being introduced in this PR.
CHANGELOG.md (1)
18-40: LGTM!The changelog accurately documents the new features, fixes, and deprecations for version 1.3.0. The deprecation messaging clearly guides users to the new small-modulus field types.
README.md (1)
80-96: LGTM!The new Lattice-Based Cryptography section provides clear, practical examples for both Kyber and Dilithium field elements, demonstrating the recommended usage pattern with the optimized small-field types.
THEORY.md (1)
1-17: LGTM!Excellent addition of comprehensive mathematical documentation. The theory document covers all the key concepts (big integers, finite fields, Montgomery multiplication, NTT, negacyclic NTT, and number theory algorithms) with clear explanations and proper LaTeX formulas.
src/poly/ntt/config.rs (2)
137-199: LGTM!The test structure is well-organized with clear separation between working small-field tests and deprecated legacy tests. The legacy tests are properly marked with
#[ignore]and clear explanations of why they fail with U1024 Montgomery arithmetic.
126-129: ROOT_OF_UNITY constant verified as correct.The value 3073009 is mathematically correct: (1753)² mod 8380417 = 3073009, and ω^256 mod 8380417 = 1 as required. No changes needed.
src/poly/ntt/mod.rs (3)
51-79: LGTM!The
NttContextstruct provides a clean API for NTT operations with well-documented fields. Precomputing ψ powers, ψ⁻¹ powers, and n⁻¹ at construction time is an excellent design choice for efficiency.
217-259: LGTM!The
mul()method now correctly clones inputs to avoid mutation, addressing the previous review concern. The documentation clearly describes the behavior, and the in-place variant is available for performance-critical paths.
261-296: LGTM!The
mul_in_place()variant is well-documented with explicit warnings about input mutation. This provides a clear API contract and gives users a choice between safety and performance.src/poly/ntt/small.rs (3)
31-161: LGTM!The
KyberFieldElementimplementation is well-designed with efficient 16-bit storage and proper Barrett reduction for the small modulus q = 3329. The API is complete with all necessary arithmetic operations, inverse, and exponentiation.
213-270: LGTM!The
DilithiumFieldElementimplementation provides a complete and well-documented API for the Dilithium modulus. The doc comment foromega()(line 266) correctly states ω = 3073009, matching the constant.
390-500: LGTM!Excellent test coverage validating arithmetic operations, primitive root properties (ζ^256 = 1 for Kyber, ψ^256 = -1 for Dilithium), inverses, and the relationship ψ² = ω. These tests provide confidence in the correctness of the implementations.
1 participant
Summary by CodeRabbit
New Features
Tests
Bug Fixes / Cleanup
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.