gate ML‑KEM/ML‑DSA specific SHA3 APIs behind optional features (#1532) - #1542
gate ML‑KEM/ML‑DSA specific SHA3 APIs behind optional features (#1532)#1542Arul-Sujith wants to merge 2 commits into
Conversation
|
Thanks for the PR! We recently added a policy on LLM-assisted submissions. Did you use any AI tooling? If yes, to what capacity? |
|
Yes, I used an LLM to assist with this PR. Primarily to understand codebase, apply fixes, and generate PR descriptions. Then manually review the fixes and run tests. I will make sure to include this disclosure upfront in future PR descriptions. |
|
Is there a reason for this PR being stacked on top of #1541 ? |
6aace21 to
285ef5f
Compare
|
That was an accident! I forgot to branch off of |
robinhundt
left a comment
There was a problem hiding this comment.
Thanks for the PR!
I don't think it is needed to have two features. In this PR, the mlkem and mldsa features are only ever used together in sha3. The cargo features should not be named by who is using them, but by what they enable. In this case, I think one "incremental" feature makes more sense, as this is the main thing consumed by ml-kem and ml-dsa.
Also note that when adding a new cargo feature, the CI tests need to be adapted so the now cfg-gated code is actually tested in CI.
It seems that kmac is also using the incremental API (see CI failure), so it also needs to enable the sha3 cargo feature.
| @@ -1,3 +1,4 @@ | |||
| #![cfg(any(feature = "mlkem", feature = "mldsa"))] | |||
There was a problem hiding this comment.
blocking: Please only cfg exclude those tests which test the functions gated by these features. As it is, none of these tests, even the portable implementation ones, are executed if neither the mlkem or mldsa feature is enabled.
| @@ -1,3 +1,4 @@ | |||
| #![cfg(any(feature = "mlkem", feature = "mldsa"))] | |||
There was a problem hiding this comment.
blocking: See cavp tests.
| [dependencies] | ||
| crabgrind.workspace = true | ||
| libcrux-sha3 = { workspace = true } | ||
| libcrux-sha3 = { workspace = true, features = ["mlkem", "mldsa"] } |
There was a problem hiding this comment.
nit: The APIs gated by these features are currently not tested in the ctgrind-test crate, so no need to enable them here.
| mlkem = [] | ||
| mldsa = [] |
There was a problem hiding this comment.
blocking: I think this can just be a single feature, e.g. "incremental".
|
Ah, one more thing. I think with the cargo feature for the incremental sha3 APIs, this will probably break the hax extraction for libcrux-ml-dsa and libcrux-ml-kem. You probably need to include the feature in the Setting up the hax toolchain is a bit of a hassle. I think the easiest option is to use the dev-shell in the flake.nix (building it the first time takes a while, as it is quite large). |
62838ea to
cebe586
Compare
cebe586 to
c5fb80f
Compare
This PR gates the ML‑KEM and ML‑DSA specific SHA3 APIs behind optional Cargo features (
mlkemandmldsa). The change cleans up the public API surface oflibcrux-sha3by hiding specialized incremental functions (e.g.,x4::incremental,x2::incremental, andportable::incremental) unless one of the new features is enabled.Changes
mlkemandmldsaoptional features tocrates/algorithms/sha3/Cargo.toml.avx2::x4,neon::x2,portable::incremental) with#[cfg(any(feature = "mlkem", feature = "mldsa"))].libcrux-ml-kem,libcrux-ml-dsa,ctgrind-test) to enable these features where needed.Fixes #1532