-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Make SHA256 compression pluggable #1777
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -134,6 +134,12 @@ SECP_TRY_APPEND_DEFAULT_CFLAGS(SECP_CFLAGS) | |
| ### Define config arguments | ||
| ### | ||
|
|
||
| AC_ARG_WITH([external-sha256], | ||
| AS_HELP_STRING([--with-external-sha256=PATH], [use external SHA256 compression implementation]), | ||
| [external_sha256_path="$withval"], | ||
| [external_sha256_path=""] | ||
| ) | ||
|
Comment on lines
+137
to
+141
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regarding the build stuff for the build-time override: We already have the possibility to have build-time overrides for the error callbacks, e.g., see When it comes to the implementation of how the new override is configured and performed, I think it's similar to the existing overrides for the callback. (Except that these don't allow configuring the header to be included, which makes them rather useless... #1461) So this will need a rework anyway [1]. This belongs to a different PR, of course, but I think the SHA override here is a good opportunity to think about a better design of a compile-time override, so we may want to iterate on this a bit more than expected. No matter how it's designed, I think we should, in the long run, have only one (non-deprecated) mechanism for a build-time override of a function. [1] I don't think what I did for the error callbacks great, and I guess it would be okay to change it (I doubt that many users rely on this). Or even better, we could deprecate it and simply provide ways to provide build-time overrides for standard library symbols like |
||
|
|
||
| # In dev mode, we enable all binaries and modules by default but individual options can still be overridden explicitly. | ||
| # Check for dev mode first because SECP_SET_DEFAULT needs enable_dev_mode set. | ||
| AC_ARG_ENABLE(dev_mode, [], [], | ||
|
|
@@ -397,6 +403,12 @@ SECP_CFLAGS="$SECP_CFLAGS $WERROR_CFLAGS" | |
|
|
||
| # Processing must be done in a reverse topological sorting of the dependency graph | ||
| # (dependent module first). | ||
|
|
||
| if test "x$external_sha256_path" != "x"; then | ||
| SECP_CONFIG_DEFINES="$SECP_CONFIG_DEFINES -DSECP256K1_EXTERNAL_SHA256=1" | ||
| SECP_CONFIG_DEFINES="$SECP_CONFIG_DEFINES -DSECP256K1_EXTERNAL_SHA256_HEADER='\"$external_sha256_path\"'" | ||
| fi | ||
|
|
||
| if test x"$enable_module_ellswift" = x"yes"; then | ||
| SECP_CONFIG_DEFINES="$SECP_CONFIG_DEFINES -DENABLE_MODULE_ELLSWIFT=1" | ||
| fi | ||
|
|
@@ -470,6 +482,7 @@ AM_CONDITIONAL([ENABLE_MODULE_EXTRAKEYS], [test x"$enable_module_extrakeys" = x" | |
| AM_CONDITIONAL([ENABLE_MODULE_SCHNORRSIG], [test x"$enable_module_schnorrsig" = x"yes"]) | ||
| AM_CONDITIONAL([ENABLE_MODULE_MUSIG], [test x"$enable_module_musig" = x"yes"]) | ||
| AM_CONDITIONAL([ENABLE_MODULE_ELLSWIFT], [test x"$enable_module_ellswift" = x"yes"]) | ||
| AM_CONDITIONAL([SECP256K1_EXTERNAL_SHA256], [test "x$SHA256_EXTERNAL_SRC" != "x"]) | ||
| AM_CONDITIONAL([USE_EXTERNAL_ASM], [test x"$enable_external_asm" = x"yes"]) | ||
| AM_CONDITIONAL([USE_ASM_ARM], [test x"$set_asm" = x"arm32"]) | ||
| AM_CONDITIONAL([BUILD_WINDOWS], [test "$build_windows" = "yes"]) | ||
|
|
@@ -494,6 +507,7 @@ echo " module extrakeys = $enable_module_extrakeys" | |
| echo " module schnorrsig = $enable_module_schnorrsig" | ||
| echo " module musig = $enable_module_musig" | ||
| echo " module ellswift = $enable_module_ellswift" | ||
| echo " external sha256 = ${external_sha256_path:-none}" | ||
| echo | ||
| echo " asm = $set_asm" | ||
| echo " ecmult window size = $set_ecmult_window" | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the motivation for exposing the SHA256 compression? Unless I'm missing something, then if anything, I think we'd want to expose the high-level SHA256 function. I'm not sure how useful it is, and we tried to keep the library focused on ECC. On the other hand, we'll have it anyway in the code base, and it's required for Schnorr sigs (and we anyway expose a tagged hash for Schnorr sigs). So if you ask me, it's fine to expose SHA256 as long as we add a comment that says that users shouldn't expect a highly performant implementation. But perhaps it's better to debate the exposing in a separate PR. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| #ifndef SECP256K1_SHA_H | ||
| #define SECP256K1_SHA_H | ||
|
|
||
| #include "secp256k1.h" | ||
|
|
||
| #include <stdint.h> | ||
|
|
||
| #ifdef __cplusplus | ||
| extern "C" { | ||
| #endif | ||
|
|
||
| /** | ||
| * SHA-256 block compression function. | ||
| * | ||
| * Performs the SHA-256 transform step on a single 64-byte message block, | ||
| * updating the 8-word `state` in place. This is the raw block-level primitive: | ||
| * no padding, no message scheduling across blocks, and no length encoding. | ||
| * Only the compression function is applied. | ||
| * | ||
| * If `rounds` is greater than 1, the same 64-byte block is re-compressed | ||
| * repeatedly onto the updated state. | ||
| * | ||
| * The caller must supply a fully-formed, 64-byte, block-aligned message block. | ||
| * | ||
| * @param state Current hash state (8 x 32-bit words), updated in place. | ||
| * @param block Pointer to a 64-byte message block. | ||
| * @param rounds Number of times to apply the compression to this block. | ||
| */ | ||
| SECP256K1_API void secp256k1_sha256_transform( | ||
| uint32_t *state, | ||
| const unsigned char *block, | ||
| size_t rounds | ||
| ) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2); | ||
|
|
||
| #ifdef __cplusplus | ||
| } | ||
| #endif | ||
|
|
||
| #endif /* SECP256K1_SHA_H */ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| include_HEADERS += include/secp256k1_sha.h | ||
| noinst_HEADERS += src/modules/sha/main_impl.h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think this one should also be an
enable-style configure option instead of awith-style configure options;withis for compiling with external packages e.g.,with-libxyz. And what the user provides here is not a package.