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

Expand test coverage to all 100 NIST KAT values #1418

Closed
dstebila opened this issue Mar 16, 2023 · 6 comments · Fixed by #1560
Closed

Expand test coverage to all 100 NIST KAT values #1418

dstebila opened this issue Mar 16, 2023 · 6 comments · Fixed by #1560
Assignees
Labels
enhancement New feature or request good first issue Issue for new contributors
Milestone

Comments

@dstebila
Copy link
Member

Currently our KAT tests only test against the first KAT, but most NIST submissions include (if memory serves) 100 values. We could do all of these.

@baentsch
Copy link
Member

baentsch commented Jul 5, 2023

We could do all of these.

The environment wouldn't thank us if we'd do it in CI (mostly wasted CPU cycles). As running it at least once does make sense, though, I'd suggest adding this at most to the weekly build/regression test, possibly only to a test script to be run manually, e.g. at release.

@dstebila
Copy link
Member Author

dstebila commented Jul 5, 2023

Agreed; weekly would certainly suffice.

@baentsch baentsch added enhancement New feature or request good first issue Issue for new contributors labels Jul 18, 2023
@SWilson4
Copy link
Member

Bumping this discussion to note that I recently encountered a subtle bug when patching HQC that only affected two of the 100 KATs. The PQClean automated tests all pass on the buggy version, since they also check only the first KAT.

@baentsch
Copy link
Member

The PQClean automated tests all pass on the buggy version, since they also check only the first KAT.

Doesn't sound good. What about raising this issue then to one to be resolved in release "0.9.0"? If the goal is to have a high certainty that release 0.9.0 indeed has proper(ly tested) implementations for all NIST finishers and R4 candidates shouldn't this issue then be resolved before that release?

@dstebila
Copy link
Member Author

It certainly would be nice. I seem to recall that when we thought about doing this several years ago the performance was rather problematic, but maybe most of the remaining algorithms it should be fine.

I don't think I'd hold the 0.9.0 release for it, but it would be good to include in our next release.

@SWilson4
Copy link
Member

SWilson4 commented Sep 21, 2023

I can take a look at expanding the kat_kem and kat_sig commands to check against all 100 KAT values. e.g. by adding an optional --full flag, and do some preliminary testing to see how feasible it is to run for all (or most, or some) of our supported algorithms.

@SWilson4 SWilson4 self-assigned this Sep 26, 2023
@dstebila dstebila added this to the 0.10.0 milestone Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Issue for new contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants