-
Notifications
You must be signed in to change notification settings - Fork 47
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
Refactor FIPS/SHA2 APIs #40
base: master
Are you sure you want to change the base?
Conversation
This is still work in progress, the accelerated implementations need to be updated as well. |
We can merge something like this for |
PQClean doesn't include optimized versions of the hash functions (yet?) --- down the line we may consider seeing if we can extract e.g. |
Everything builds so it seems mostly ready for merging on my end. The diff is slightly on the large side, but the main things I'm doing is moving non-hashscheme-specific code out of e.g. |
You want a review? |
Yep, please let me know what you think or if there's any things that could better be refactored into a different file or something like that. |
Merging this set of patches would make it a lot easier to go forward with the downstream updates in PQClean and liboqs. I would appreciate it if it could get some eyes. If it helps I can probably try to clarify what kind of changes I made, let me know if that'd help. |
@thomwiggers Can you do before/after benchmarks? I’ll take a closer look this week. (I have some more time now :). ) |
I've ran the benchmarks, and comparing them side-by-side using |
Hi Bas, gentle nudge 🙂 |
You add a lot of dead code, eg. sha3_384. I'm not so sure that the benchmarks show it doesn't have any impact. Let me double check. |
There seems to be a small but significant difference for signing and keygen with shake.
|
I'm not quite sure where the difference comes from. For the |
Yup, also some small but significant slowdowns:
|
Cleaning up the dead code is obviously something that we can easily do (and makes sense to do). We should also probably clear up the thing Douglas pointed out; I was just waiting on a bit of feedback so I could batch the polishing work. Those differences in the hashing performance are interesting though. It's funny that it only seems to be happening (or was only statistically significant?) in ref's Meanwhile in SHA2-AVX2, the most significant code changes seem to be the addition of the Can you share the script you wrote to generate those statistical comparisons? I should probably measure this again with a few different compilers rather than just GCC 12, and in a cleaner environment than a development VM. |
https://gist.github.com/bwesterb/db8083608aeb4161021a60eeeb84fe71 Pipe the output of
I pinned CPU frequencies to something low and disabled turbo boost for accurate results. |
I've run a lot more benchmarks, with 50 measurements instead of 10, and running them for Clang and GCC with and without LTO. I'm finally getting around to looking at the results that I generated ages ago. I'm finding the following interesting results:
|
Yes, this could very well be true. I'm now leaning towards merging something like this PR. However, the diff is too big and contains a lot of noise: for instance, I don't really care that you renamed |
I'm trying to sort out the changes into separate commits as much as possible. Hopefully that will help. |
d7eef62
to
d6b08cf
Compare
* Use abstract types for SHA2 state This allows to more easily replace SHA2 implementations * Define 'free'-style functions for hash state This allows potential heap-based SHA2 implementations to instantiate sha2 in SPHINCS+
d6b08cf
to
d386a73
Compare
* Uses opaque incremental hashing context Allows easier replacement of hashing primitives by different backing implementations. * Adds context release functions Allows heap-backed FIPS202 implementations. fips202.[ch] from PQClean
d386a73
to
bafee15
Compare
I think I did as much as I could do without re-applying changes by hand. You should now be able to review the individual commits and they should mostly make sense on their own. Let me know if you really would like some stuff split out. I can also continue working on the integration into PQClean before we merge this PR; that will result in applying lots and lots of more testing on a bunch of different platforms, linters and compilers. While that will lead to more changes (notably MSVC is REALLY picky about implicit downcasting) it will also confirm if everything is working. |
clang-tidy suggested that this layout was more efficient.
Could you rebase? Let's see if the builds succeed then. |
Also, it seems you didn't restructure the patch set yet for easy review. Right? |
I did, but I found a few things in the process of preparing the PQClean versions. |
This allows to more easily replace SHA2/SHAKE implementations
This allows potential heap-based SHA2/SHAKE implementations to instantiate those functions in SPHINCS+
Updates fips202.c/sha2.c implementations based on those found in PQClean (which are based on a common ancestor anyway).