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

Add ML-KEM KATs #150

Merged
merged 6 commits into from
Oct 4, 2024
Merged

Add ML-KEM KATs #150

merged 6 commits into from
Oct 4, 2024

Conversation

marsella
Copy link
Contributor

@marsella marsella commented Oct 3, 2024

Closes #148

This adds a few easy-to-run tests for ML-KEM that will hopefully make it easier to be confident about all the upcoming changes in the ML-KEM gold standard issues. They are not fast and I only added one for each parameter set, although there are many more in the sourced repo.

In addition to the work listed in the linked issue, this also renames all the modules around here to adhere better to our standards.

@sauclovian-g
Copy link

I would strongly recommend doing just the git rename ops as one commit, and putting all the content changes in a second separate one, even though that probably means the first commit won't be buildable. Otherwise you're at the mercy of git's rename detection heuristics. (Note that if you look at the first commit here, it mostly comes up as deletes and entirely new files; that's because the rename detection heuristics don't work very well. 😐 This means for example that you won't be able to cross the rename with git log or git blame.)

At this point that'll be kind of a pain to retcon in though and maybe it's not really worthwhile...

This fixes files to be capitalized, removes an illegal dash, and adjusts
things to sit in our newly established directory structure. Note that
this does not build because the module names no longer match the file
names.
This brings module names into alignment with our standard (capitalized,
matches the directory path, etc) and the directory structure in line
with our recent changes (`Instantiations/` and `Tests/`)
@marsella
Copy link
Contributor Author

marsella commented Oct 4, 2024

I would strongly recommend doing just the git rename ops as one commit, and putting all the content changes in a second separate one, even though that probably means the first commit won't be buildable. Otherwise you're at the mercy of git's rename detection heuristics. (Note that if you look at the first commit here, it mostly comes up as deletes and entirely new files; that's because the rename detection heuristics don't work very well. 😐 This means for example that you won't be able to cross the rename with git log or git blame.)

At this point that'll be kind of a pain to retcon in though and maybe it's not really worthwhile...

It didn't take that long to fix, so now the individual commit diffs look good. The overall diff still fails to recognize the rename, but hopefully this will help anyone pawing through the history.

@marsella marsella merged commit 972b6a7 into master Oct 4, 2024
2 checks passed
@marsella marsella deleted the 148-add-mlkem-kats branch October 4, 2024 14:21
@sauclovian-g
Copy link

It will. I just checked and git log --follow works, and it probably didn't before. Also should anyone be merging/rebasing they can now merge stepwise over the renames if they need to, which can avoid all kinds of weird problems.

Thanks :-)

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.

Add easy-to-run KATs to ML-KEM
3 participants