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

Integrate Vectorized SHA3 #433

Merged
merged 6 commits into from
Dec 4, 2023
Merged

Integrate Vectorized SHA3 #433

merged 6 commits into from
Dec 4, 2023

Conversation

mamonet
Copy link
Member

@mamonet mamonet commented Dec 1, 2023

This patch imports Hacl_SHA3_Scalar and Hacl_SHA3_Vec256 modules from hacl-star sha3-mb branch and adds sha3-mb.cc test file to test SHA-3 modes for single-lane and 4-lanes.

@mamonet mamonet requested a review from a team as a code owner December 1, 2023 11:17
@cla-bot cla-bot bot added the cla-signed label Dec 1, 2023
@coveralls
Copy link

coveralls commented Dec 1, 2023

Pull Request Test Coverage Report for Build 7084464025

  • 9842 of 12270 (80.21%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+5.0%) to 59.089%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/Hacl_SHA3_Scalar.c 1986 2304 86.2%
src/Hacl_SHA3_Vec256.c 7856 9966 78.83%
Totals Coverage Status
Change from base Build 7058563693: 5.0%
Covered Lines: 37671
Relevant Lines: 63753

💛 - Coveralls

Copy link

@karthik-cryspen karthik-cryspen left a comment

Choose a reason for hiding this comment

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

Does this ever test the vec256 version with 4 different inputs? If not, would be worth adding such a test , or trying with 4 different kats, 4 at a time.

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.

I didn't look much at the C code. I presume that's fine.

Generally looks good. Only a few things inline.

Also, the MSVC code appears to be missing. Please add that. The other bindings aren't too important yet as long as the CI is happy with it.

tests/sha3-mb.cc Outdated Show resolved Hide resolved
tests/sha3-mb.cc Outdated Show resolved Hide resolved
tests/sha3-mb.cc Outdated Show resolved Hide resolved
@franziskuskiefer
Copy link
Member

I fixed the examples on main, please cherry pick that commit or do another PR to merge main into dev first to fix that here.

@mamonet
Copy link
Member Author

mamonet commented Dec 4, 2023

Does this ever test the vec256 version with 4 different inputs? If not, would be worth adding such a test , or trying with 4 different kats, 4 at a time.

I used 4 different inputs for one shake128 vec256 test.

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.

Thanks.
Please run clang-format on the cc files. Then this is good to go. There are some types in the generated headers that we should update, but we can do that in another iteration.

include/msvc/Hacl_SHA3_Vec256.h Show resolved Hide resolved
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.

Thanks!

@franziskuskiefer franziskuskiefer merged commit 6b66fae into cryspen:dev Dec 4, 2023
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants