On Tue, 8 Oct 2024 18:16:47 GMT, Ben Perez <bpe...@openjdk.org> wrote:
>> Java implementation of ML-DSA, the FIPS 204 post-quantum signature scheme >> https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.204.pdf. Depends on >> https://github.com/openjdk/jdk/pull/21167 > > Ben Perez has updated the pull request incrementally with one additional > commit since the last revision: > > renamed internal keyGen/sign/verify functions to be same as spec Another comment: I noticed you didn't `mod q` everywhere. The result should be equivalent and I hope they are always "normalized" at encoding. src/java.base/share/classes/sun/security/provider/ML_DSA.java line 35: > 33: > 34: public class ML_DSA { > 35: private static final int mlDsa_q = 8380417; `static final` constants are usually in all UPPER_CASE in Java. Also, `t0CoeffSize` seems also a constant. src/java.base/share/classes/sun/security/provider/ML_DSA.java line 706: > 704: } > 705: } > 706: } Add a comment here that `shift` must be zero now so we should have output all bits. src/java.base/share/classes/sun/security/provider/ML_DSA.java line 710: > 708: } > 709: > 710: public int[][] t1Unpack(byte[] v) { Maybe worth saying this is `SimpleBitUnpack` always with the same argument. src/java.base/share/classes/sun/security/provider/ML_DSA.java line 811: > 809: public ML_DSA_PublicKey pkDecode(byte[] pk) { > 810: byte[] rho = new byte[mlDsaASeedLength]; > 811: System.arraycopy(pk, 0, rho, 0, mlDsaASeedLength); Maybe use `Arrays.copyOfRange` to make it more clear. src/java.base/share/classes/sun/security/provider/ML_DSA.java line 916: > 914: this.xof = xof; > 915: this.bitsPerCall = bitsPerCall; > 916: bitMask = (1 << bitsPerCall) - 1; Add some comments about the limit of `bitsPerCall`, looks like cannot exceed 31. src/java.base/share/classes/sun/security/provider/ML_DSA.java line 1174: > 1172: int result = implMlDsaAlmostNtt(coeffs, montZetasForVectorNtt); > 1173: int[] check = coeffs.clone(); > 1174: result = implMlDsaMontMulByConstant(coeffs, montRModQ); In FIPS 204, NTT does not end with multiplying a constant. Why do you need one? src/java.base/share/classes/sun/security/provider/ML_DSA.java line 1202: > 1200: public static int[] mlDsaInverseNtt(int[] coeffs) { > 1201: int result = implMlDsaAlmostInverseNtt(coeffs, > montZetasForVectorInverseNtt); > 1202: result = implMlDsaMontMulByConstant(coeffs, montDimInverse); In FIPS 204, the constant is 8347681. Why do you use 16382? ------------- PR Review: https://git.openjdk.org/jdk/pull/21364#pullrequestreview-2360756958 PR Review Comment: https://git.openjdk.org/jdk/pull/21364#discussion_r1795691383 PR Review Comment: https://git.openjdk.org/jdk/pull/21364#discussion_r1795694178 PR Review Comment: https://git.openjdk.org/jdk/pull/21364#discussion_r1795695028 PR Review Comment: https://git.openjdk.org/jdk/pull/21364#discussion_r1795695988 PR Review Comment: https://git.openjdk.org/jdk/pull/21364#discussion_r1795697317 PR Review Comment: https://git.openjdk.org/jdk/pull/21364#discussion_r1795700210 PR Review Comment: https://git.openjdk.org/jdk/pull/21364#discussion_r1795702588
