ML-DSA unique names #2072

jakemas · 2024-12-20T02:55:39Z

Issues:

As we prepare to move ML-DSA to the FIPS module we encounter many issues with duplicate definitions due to the similarities between ML-DSA and ML-KEM (that already exists within the module).

To address this issue we remove these duplicate names, and replace with more specific naming. This aides with code readability.

Description of changes:

Renamed:

N is now ML_DSA_N
Q is now ML_DSA_Q
D is now ML_DSA_D
QINV is now ML_DSA_QINV
MONT, ROOT_OF_UNITY are never used, so we remove them.
DILITHIUM_K_MAX is now ML_DSA_K_MAX
DILITHIUM_L_MAX is now ML_DSA_L_MAX
DILITHIUM_C_TILDE_BYTES_MAX is now ML_DSA_C_TILDE_BYTES_MAX
DILITHIUM_POLYW1_PACKEDBYTES_MAX is now ML_DSA_POLYW1_PACKEDBYTES_MAX
DILITHIUM_POLY_UNIFORM_ETA_NBLOCKS_MAX is now ML_DSA_POLY_UNIFORM_ETA_NBLOCKS_MAX
DILITHIUM_POLYZ_PACKEDBYTES_MAX is now ML_DSA_POLYZ_PACKEDBYTES_MAX
poly_reduce is now ml_dsa_poly_reduce
poly_add is now ml_dsa_poly_add
poly_sub is now ml_dsa_poly_sub
poly_ntt is now ml_dsa_poly_ntt
poly_invntt_tomont is now ml_dsa_poly_invntt_tomont
crypto_sign_keypair is now ml_dsa_keypair
crypto_sign_keypair_internal is now ml_dsa_keypair_internal
crypto_sign_signature is now ml_dsa_signa
crypto_sign_signature_internal is now ml_dsa_sign_internal
crypto_sign is now ml_dsa_sign_message
crypto_sign_verify is now ml_dsa_verify
crypto_sign_verify_internal is now ml_dsa_verify_internal
crypto_sign_open is now ml_dsa_verify_message
fqmul is now ml_dsa_fqmul
invntt_tomont is now ml_dsa_invntt_tomont
pack_pk is now ml_dsa_pack_pk
unpack_pk is now ml_dsa_unpack_pk
pack_sk is now ml_dsa_pack_sk
unpack_sk is now ml_dsa_unpack_sk
polyeta_pack is now ml_dsa_polyeta_pack
polyeta_unpack is now ml_dsa_polyeta_unpack
polyt0_pack is now ml_dsa_polyt0_pack
polyt0_unpack is now ml_dsa_polyt0_unpack
pack_sig is now ml_dsa_pack_sig
unpack_sig is now ml_dsa_unpack_sig
polyz_pack is now ml_dsa_polyz_pack
polyz_unpack is now ml_dsa_polyz_unpack
SEEDBYTES is now ML_DSA_SEEDBYTES
TRBYTES is now ML_DSA_TRBYTES
POLYT0_PACKEDBYTES is now ML_DSA_POLYT0_PACKEDBYTES
POLYT1_PACKEDBYTES is now ML_DSA_POLYT1_PACKEDBYTES
RNDBYTES is now ML_DSA_RNDBYTES
poly_caddq is now ml_dsa_poly_caddq
poly_shiftl is now ml_dsa_poly_shiftl
poly_pointwise_montgomery is now ml_dsa_poly_pointwise_montgomery
poly_power2round is now ml_dsa_poly_power2round
poly_decompose is now ml_dsa_poly_decompose
poly_make_hint is now ml_dsa_poly_make_hint
poly_use_hint is now ml_dsa_poly_use_hint
poly_chknorm is now ml_dsa_poly_chknorm
poly_uniform is now ml_dsa_poly_uniform
poly_uniform_eta is now ml_dsa_poly_uniform_eta
poly_uniform_gamma1 is now ml_dsa_poly_uniform_gamma1
poly_challenge is now ml_dsa_poly_challenge
polyeta_pack is now ml_dsa_polyeta_pack
polyeta_unpack is now ml_dsa_polyeta_unpack
polyt1_pack is now ml_dsa_polyt1_pack
polyt1_unpack is now ml_dsa_polyt1_unpack
polyt0_pack is now ml_dsa_polyt0_pack
polyt0_unpack is now ml_dsa_polyt0_unpack
polyz_pack is now ml_dsa_polyz_pack
polyz_unpack is now ml_dsa_polyz_unpack
polyw1_pack is now ml_dsa_polyw1_pack
polyvecl_uniform_eta is now ml_dsa_polyvecl_uniform_eta
polyvecl_uniform_gamma1 is now ml_dsa_polyvecl_uniform_gamma1
polyvecl_reduce is now ml_dsa_polyvecl_reduce
polyvecl_add is now ml_dsa_polyvecl_add
polyvecl_ntt is now ml_dsa_polyvecl_ntt
polyvecl_invntt_tomont is now ml_dsa_polyvecl_invntt_tomont
polyvecl_pointwise_poly_montgomery is now ml_dsa_polyvecl_pointwise_poly_montgomery
polyvecl_pointwise_acc_montgomery is now ml_dsa_polyvecl_pointwise_acc_montgomery
polyvecl_chknorm is now ml_dsa_polyvecl_chknorm
polyveck_uniform_eta is now ml_dsa_polyveck_uniform_eta
polyveck_reduce is now ml_dsa_polyveck_reduce
polyveck_caddq is now ml_dsa_polyveck_caddq
polyveck_add is now ml_dsa_polyveck_add
polyveck_sub is now ml_dsa_polyveck_sub
polyveck_shiftl is now `ml_dsa_polyveck_shiftl'
polyveck_ntt is now ml_dsa_polyveck_ntt
polyveck_invntt_tomont is now ml_dsa_polyveck_invntt_tomont
polyveck_pointwise_poly_montgomery is now ml_dsa_polyveck_pointwise_poly_montgomery
polyveck_chknorm is now ml_dsa_polyveck_chknorm
polyveck_power2round is now ml_dsa_polyveck_power2round
polyveck_decompose is now ml_dsa_polyveck_decompose
polyveck_make_hint is now ml_dsa_polyveck_make_hint
polyveck_use_hint is now ml_dsa_polyveck_use_hint
polyveck_pack_w1 is now ml_dsa_polyveck_pack_w1
polyvec_matrix_expand is now ml_dsa_polyvec_matrix_expand
polyvec_matrix_pointwise_montgomery is now ml_dsa_polyvec_matrix_pointwise_montgomery
reduce32 is now ml_dsa_reduce32
caddq is now ml_dsa_caddq
freeze is now ml_dsa_freeze
power2round is now ml_dsa_power2round
decompose is now ml_dsa_decompose
make_hint is now ml_dsa_make_hint
use_hint is now ml_dsa_use_hint
poly is now ml_dsa_poly
rej_uniform is now ml_dsa_rej_uniform

(the crypto_sign_* names are an artefact of the NIST PQC submission requirements, so are replaced with meaningful names)

Call-outs:

Only changed names where necessary.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

codecov-commenter · 2024-12-20T03:19:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.76%. Comparing base (39b3fae) to head (533e78b).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2072   +/-   ##
=======================================
  Coverage   78.75%   78.76%           
=======================================
  Files         598      598           
  Lines      103650   103650           
  Branches    14718    14719    +1     
=======================================
+ Hits        81633    81640    +7     
+ Misses      21366    21359    -7     
  Partials      651      651

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jakemas · 2024-12-20T16:26:31Z

crypto/dilithium/pqcrystals_dilithium_ref_common/poly.h

 } poly;

-void poly_reduce(poly *a);
+void ml_dsa_poly_reduce(poly *a);

 void poly_caddq(poly *a);


Could prefix these for consistency, but also not necessary and deeply internal. The ones changed are the ones that conflict with existing functions in the FIPS module (i.e., ML-KEM got there first). Functions are not equivalent to ML-KEM variants as they often have differing parameter set (for example the zetas in ntt).

I prefer prefix everything for consistency and avoiding any future confusion.

done 2bbdfc9 for everything

andrewhop · 2024-12-23T20:26:58Z

crypto/dilithium/pqcrystals_dilithium_ref_common/params.h

@@ -9,10 +9,9 @@
 #define CRHBYTES 64
 #define TRBYTES 64
 #define RNDBYTES 32


Even thought you didn't hit a conflict can you also rename all the other defines, I could see seedbytes/rndbytes potentially conflicting or at least confusing someone else in the future

done 2bbdfc9

andrewhop · 2024-12-23T20:30:38Z

crypto/dilithium/pqcrystals_dilithium_ref_common/sign.h

@@ -5,48 +5,48 @@
 #include <stdint.h>
 #include "params.h"

-int crypto_sign_keypair(ml_dsa_params *params, uint8_t *pk, uint8_t *sk);
+int mldsa_keypair(ml_dsa_params *params, uint8_t *pk, uint8_t *sk);


Missed renaming the header file include guard on line 1

andrewhop · 2024-12-23T20:33:09Z

crypto/dilithium/pqcrystals_dilithium_ref_common/poly.h

 } poly;

-void poly_reduce(poly *a);
+void ml_dsa_poly_reduce(poly *a);

 void poly_caddq(poly *a);


I prefer prefix everything for consistency and avoiding any future confusion.

andrewhop · 2024-12-23T20:37:40Z

crypto/dilithium/pqcrystals_dilithium_ref_common/packing.c

@@ -208,7 +208,7 @@ void pack_sig(ml_dsa_params *params,

  k = 0;
  for(i = 0; i < params->k; ++i) {
-    for(j = 0; j < N; ++j) {
+    for(j = 0; j < ML_DSA_N; ++j) {


Missed updating the header file guard in packing.h

andrewhop · 2024-12-23T20:38:11Z

crypto/dilithium/pqcrystals_dilithium_ref_common/rounding.c

@@ -18,8 +18,8 @@
 int32_t power2round(int32_t *a0, int32_t a)  {
  int32_t a1;

-  a1 = (a + (1 << (D-1)) - 1) >> D;
-  *a0 = a - (a1 << D);
+  a1 = (a + (1 << (ML_DSA_D-1)) - 1) >> ML_DSA_D;


Please update rounding.h

done in 2bbdfc9 (I also missed packing.h

geedo0

LGTM. One thing that I find helpful in these kinds of big refactors is to describe the process you used to perform them. This gives the reviewers some hints around what to look out for. e.g. Did you use CLion's introspective refactoring tool or was it a generic search/replace tool?

Other note, what's your thinking around the relationship between this fork and the upstream reference? At this point, we're getting diverged enough that merge conflicts are expected. Is the upstream effectively dead and we don't expect to ever have to pull in upstream changes? Being aligned on what that relationship will be going forward would help guide decisions around how this code should be maintained.

jakemas · 2024-12-30T18:33:42Z

Name replacement was done manually using find and replace.

I agree that we have significantly diverged from the reference implementation. However, fundamentally the algorithm implementation is the same, while patching from upstream is now more arduous, it is certainly still simple to apply any changes from upstream. While I'm not certain of the reference implementations future plans, I would imagine that it would likely be updated with any significant (implementation based) finding.

rod-chapman · 2024-12-31T02:51:46Z

Please talk to the maintainers of mlkem-native to make sure that these proposed change are consistent and do not clash with the naming structure of their repo. There may be code and/or constants that are common to both MLKEM and MLDSA.

…2078) ### Description of changes: Inspired by the issues in #2072 this PR update's ML-KEM's include guards to use unique names to avoid any future issues. ### Call-outs: Future PRs could also prefix more of the functions. ### Testing: Existing tests will cover any issues. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

jakemas added 3 commits December 19, 2024 18:40

header collisions

5084da9

ml_dsa unique constant names

f420d8f

rename duplicate functions

a3fd830

jakemas requested a review from a team as a code owner December 20, 2024 02:55

rename dilithium constants

4a7a61a

jakemas mentioned this pull request Dec 20, 2024

Remove ENABLE_DILITHIUM flag #2070

Closed

change generic names to mldsa specific

363d631

jakemas commented Dec 20, 2024

View reviewed changes

andrewhop reviewed Dec 23, 2024

View reviewed changes

andrewhop mentioned this pull request Dec 23, 2024

Update ML-KEM's internal header files to use unique include guards #2078

Merged

jakemas added 4 commits December 23, 2024 19:49

added prefix for all mldsa functions to prevent conflicts

2bbdfc9

Merge branch 'main' into ml-dsa-unique-names

ae01f07

unique names for poly

f96096f

consistency in naming

436dde3

nebeid previously approved these changes Dec 24, 2024

View reviewed changes

renamed signature to sign

88d6602

jakemas dismissed nebeid’s stale review via 88d6602 December 24, 2024 21:48

nebeid approved these changes Dec 24, 2024

View reviewed changes

This was referenced Dec 26, 2024

Remove enable_dilithium flag #2081

Closed

Remove ENABLE_DILITHIUM flag #2082

Closed

Merge branch 'main' into ml-dsa-unique-names

533e78b

geedo0 approved these changes Dec 30, 2024

View reviewed changes

justsmth merged commit 4ca10b9 into aws:main Dec 31, 2024
125 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ML-DSA unique names #2072

ML-DSA unique names #2072

jakemas commented Dec 20, 2024 •

edited

Loading

codecov-commenter commented Dec 20, 2024 •

edited

Loading

jakemas Dec 20, 2024 •

edited

Loading

andrewhop Dec 23, 2024

jakemas Dec 24, 2024

andrewhop Dec 23, 2024

jakemas Dec 24, 2024

andrewhop Dec 23, 2024

jakemas Dec 24, 2024

andrewhop Dec 23, 2024

andrewhop Dec 23, 2024

jakemas Dec 24, 2024

andrewhop Dec 23, 2024

jakemas Dec 24, 2024

geedo0 left a comment

jakemas commented Dec 30, 2024

rod-chapman commented Dec 31, 2024

ML-DSA unique names #2072

ML-DSA unique names #2072

Conversation

jakemas commented Dec 20, 2024 • edited Loading

Issues:

Description of changes:

Call-outs:

codecov-commenter commented Dec 20, 2024 • edited Loading

Codecov Report

jakemas Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geedo0 left a comment

Choose a reason for hiding this comment

jakemas commented Dec 30, 2024

rod-chapman commented Dec 31, 2024

jakemas commented Dec 20, 2024 •

edited

Loading

codecov-commenter commented Dec 20, 2024 •

edited

Loading

jakemas Dec 20, 2024 •

edited

Loading