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

C extraction for ML-DSA #710

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

C extraction for ML-DSA #710

wants to merge 26 commits into from

Conversation

franziskuskiefer
Copy link
Member

@franziskuskiefer franziskuskiefer commented Dec 9, 2024

This PR makes all necessary to extract C code from ML-DSA.

  • It updates the F* code that still laxes.
  • It includes a minimal self test.

To generate the C code, run

./c.sh --config cg.yaml --out cg --no-glue --no-unrolling --no-karamel_include --no-karamel_include --mldsa65

Follow ups

  • The generated code right now needs to be edited to remove the empty structs (see Drop (empty) unused structs AeneasVerif/eurydice#107).
  • The C boilerplate, tests, benchmarks need to be cleaned up in a follow up.
  • The avx2 variant may still have issues.
  • The stack size used by the C code is too large.

Note that this PR is on top of #707.
This works towards #706 but doesn't close it yet (see follow ups).

@franziskuskiefer franziskuskiefer requested a review from a team as a code owner December 9, 2024 10:27
@franziskuskiefer franziskuskiefer self-assigned this Dec 9, 2024
keks
keks previously requested changes Dec 10, 2024
Copy link
Member

@keks keks left a comment

Choose a reason for hiding this comment

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

Some of my comments are nits, some a questions, but I think by and large this isn't controversial :)

libcrux-sha3/src/lib.rs Outdated Show resolved Hide resolved
}

impl XofAbsorb<168> for Shake128Absorb {
type Squeeze = Shake128Squeeze;
impl Xof<168> for Shake128Xof {
Copy link
Member

Choose a reason for hiding this comment

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

Should these impls somehow prevent using this sponge in a duplex way (i.e. absorbing after squeezing)? My understanding is that a duplex is not an xof

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what it did before. But I don't think we should do that here. It's only creating more boilerplate when using it. This is not a general purpose sha3 implementation right now that people should be using (unless they know what they are doing).

Copy link
Member

Choose a reason for hiding this comment

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

They did that on a type level, I was just thinking of setting a bit when squeezing and then panicking when trying to squeeze. But I hear you wrt. not a general purpose SHA3, so it should be fine, as long as we know that we use it correctly. Does that follow from our proofs?

Comment on lines -7 to +8
pub(crate) trait Xof {
/// An ML-DSA specific Xof trait
pub(crate) trait DsaXof {
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference betwen DsaXof and Xof?

Copy link
Member Author

Choose a reason for hiding this comment

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

This (DsaXof) is not actually a full Xof implementation but opererates only on multiple of blocks. That's enough for the algorithm. But not enough to hash the messages. That's where we need a proper Xof. That's why we needed to add a proper Xof here for DSA. This needs to get cleaned up in sha3. As part of that we should clean up names here too.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Can we add that as a comment?

libcrux-ml-dsa/src/hash_functions.rs Show resolved Hide resolved
libcrux-ml-dsa/src/ml_dsa_generic/instantiations/avx2.rs Outdated Show resolved Hide resolved
libcrux-ml-dsa/src/encoding/t1.rs Outdated Show resolved Hide resolved
libcrux-ml-dsa/src/matrix.rs Show resolved Hide resolved
Base automatically changed from franziskus/mldsa-c1 to main December 10, 2024 16:34
@github-actions github-actions bot dismissed keks’s stale review December 11, 2024 10:09

Review re-requested

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants