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

Expose the SHAKE-128 incremental API in libcrux and add 100,000 Kyber KAT tests using this API #156

Draft
wants to merge 24 commits into
base: dev
Choose a base branch
from

Conversation

xvzcf
Copy link
Contributor

@xvzcf xvzcf commented Jan 3, 2024

No description provided.

Copy link
Member

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

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

Generally looks good. A couple comments.

@@ -1004,462 +998,3 @@ extern "C" {
key_len: u32,
);
}
extern "C" {
Copy link
Member

Choose a reason for hiding this comment

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

To regenerate the bindings correctly you need to build for an x64 target. That's the only one that has all the features.

.allowlist_function("Hacl_Hash_.*")
.allowlist_function("Hacl_Blake2.*")
.allowlist_function("Hacl_Hash_SHA3.*")
.allowlist_function("Hacl_Hash_Blake2.*")
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 reason to list all of these explicitely now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it would just document better what exactly we want and what we don't want. Also, this let me exclude Hacl_Hash_SHA3_Scalar_sha3*, Hacl_Hash_SHA3_Scalar_shake128 and the like from the extraction (they weren't being used and I didn't want to add them in).

tests/kyber_kats/generate_shake128_kats_hash.py Outdated Show resolved Hide resolved
tests/kyber_shake_kats.rs Outdated Show resolved Hide resolved
src/hacl/sha3.rs Show resolved Hide resolved
src/hacl/sha3.rs Show resolved Hide resolved
src/hacl/sha3.rs Outdated Show resolved Hide resolved
@franziskuskiefer
Copy link
Member

This is blocked on fixing the includes for avx2 in cryspen/hacl-packages#443.

@karthikbhargavan
Copy link
Contributor

Is this PR still alive @xvzcf?
It seems we already did this elsewhere.

@xvzcf
Copy link
Contributor Author

xvzcf commented Jun 26, 2024

Is this PR still alive @xvzcf? It seems we already did this elsewhere.

I think the SHA3 incremental API is exposed, but I don't think we have the 100k KAT test, I can open a fresh PR adding that.

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

Successfully merging this pull request may close these issues.

3 participants