-
Notifications
You must be signed in to change notification settings - Fork 503
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
Fix HQC Performance - Draft #2054
Conversation
- add hqc-submission_2023-04-30 files for all hqc variants
Thank you very much @BartBBM to "(re)start from first principles" with this PR. To me, this might be indicative of something being "seriously fishy" either with the common code and/or its integration. Regarding this PR, I'm not sure this is the right approach, though (to not use the shared code): While this solves the problem for HQC, it may hide a bigger problem of OQS... |
In March I may have time to tidy this code and look further into it. In the mean time I just wanted to put it here for people to see, not for liboqs to adopt. Though to give a heads up, a small comparison between kilo cpu cycles stated by the authors and the cpu cycles needed by liboqs: hqc128 of PR: So it is not as bad as it may seem. |
Can I ask how you did the cycle count @BartBBM ? Also, is the comparison implying that |
I just ran the
Yes. but 8% off for hqc that this draft supposes, not for liboqs Taking my numbers in the initial description of this PR leads us to the value for liboqs I meant it is not that bad for bike, since a slowdown of 2 is not really noticable in a wider context, the slowdown of 36 for hqc in
|
Thanks for the additional explanation @BartBBM . I originally did not correctly latch on to the baseline, that you compared the PR's HQC performance with the author's cycle counts -- where 8% I guess is within the margin of error -- as this PR is pretty much exactly using the original code, that's reasonably to be expected, right?
Hmm -- would you agree one could also consider this 100% worse than expected? Admitted, not as bad as 3500% off :} but still pretty undesirable and worth while a serious investigation where this performance gets lost, no? |
Thanks for the benchmarking comparisons. I agree that the performance issues should be investigated, but please bear the following in mind.
Please be advised that the current reference implementation of HQC has a serious correctness and security flaw that has not been patched in the official implementation from https://pqc-hqc.org.
Unfortunately, it does not "work" in the sense that it does not correctly handle invalid decapsulation inputs—as stated in the security advisory linked above. Even for correct inputs, the reference code mishandles the private key. |
Thank you @SWilson4 for bringing this to my attention. |
In the light of the latest conversation, would it be OK to close this PR then, @BartBBM ? I don't envision you'd rather use it at high performance but with security problems, right? @SWilson4 shall we create a separate issue (eventually regression-test PR) for general performance "sanity checks"? Also, apologies for completely forgetting that you updated only the immediate upstream in #2026 and not the original HQC code base :-( --> What about adding more explicit wording for each algorithm as to its "full" maintenance status (including all direct and indirect upstreams)? In this example it might read "HQC code unmaintained at code origin, maintained at best-effort basis at direct upstream PQClean and integrated without patch to liboqs" (or so). |
It's fine for me to close it, as it is only for my performace comparisons, not for real world applications. As I said, I might revisit the performance issue of liboqs in a month. For Context: The security patch referenced by @SWilson4 seems simple and should not result in a performance loss for this hqc implementation. |
FYI @BartBBM, I took a look at the performance degradation in more detail here: #2047 (comment) If you do revisit this, I would be more than happy to support/review any work to improve HQC (while maintaining security, of course). |
This PR shows that a fix for #2047 is possible, bringing the performance of the
keygen
,encaps
anddecaps
from miliseconds again to submiliseconds, improving the performance on my machine by one to two orders of magnitude (see some numbers below).This PR is only a draft PR focusing on the performance side of things. I do not know if this code is secure.
This PR is for people who need a quick fix for the performance of HQC in liboqs (like me :D ).
What I have done
What this PR lacks
Some Numbers
old hqc-128
new hqc-128
old hqc-192
new hqc-192
old hqc-256
new hqc-256