Make SHA256 compression runtime pluggable

[furszy]

Tackling the long-standing request #702.

Right now we ship our own SHA256 implementation, a standard baseline version that does not take advantage of any hardware-optimized instruction, and it cannot be accessed by the embedding application - it is for internal usage only.

This means embedding applications often have to implement or include a different version for their use cases, wasting space on constrained environments, and in performance-sensitive setups it forces them to use a slower path than what the platform provides. Many projects already rely on tuned SHA-NI / ARMv8 / or other hardware-optimized code, so always using the baseline implementation we ship within the library is not ideal.

These changes allow users to supply their own SHA256 compression function at runtime, while preserving the existing default behavior for everyone else. This is primarily intended for environments where the available SHA256 implementation is detected dynamically and recompiling the library with a different implementation is not feasible (equivalent build-time functionality will come in a follow-up PR).

It introduces a new API:

secp256k1_context_set_sha256_transform_callback(ctx, fn_transform)

This function installs the optimized SHA256 compression into the secp256k1_context, which is then used by all internal computations. Important: The provided function is verified to be output-equivalent to the original one.

As a quick example, using this functionality in Bitcoin-Core will be very straightforward: furszy/bitcoin-core@f68bef0

[fjahr]

I guess this is more of a draft for initial review but I will spell it out anyway: It's currently missing some CI coverage for building with an external library as well as some docs. Curious what people think in terms of docs for something like this: Should there be extensive guidance in which context this is safe to use or would users be left on their own to try it out? Or are we assuming all users that go to this length are competent enough to judge if it's a good idea to bring their own sha256 or use the systems one?

[furszy]

Thanks for the feedback fjahr!

I guess this is more of a draft for initial review but I will spell it out anyway: It's currently missing some CI coverage for building with an external library as well as some docs.

Yeah, can create a CI job testing the compile-time pluggable compression function very easily. Thanks for reminding that.

And about the missing docs; yeah. I didn't add it because we currently don't have much documentation outside configure.ac / CMakeLists.txt. Happy to create a file for it.

Curious what people think in terms of docs for something like this: Should there be extensive guidance in which context this is safe to use or would users be left on their own to try it out? Or are we assuming all users that go to this length are competent enough to judge if it's a good idea to bring their own sha256 or use the systems one?

It’s not that we’re letting people plug in whatever they want. Both introduced features have guardrails:

1. Compile-time: we run all current tests against the provided implementation, plus a runtime self-test.
2. Runtime: besides the runtime self-test, have introduced an "equivalence check" that hashes known inputs and compares them against the library’s internal implementation before accepting the external function.

So you can bring your own compression function, but it still has to prove it behaves exactly like ours bit-for-bit.

Also, the target user here is someone who's actually written a hardware-optimized SHA256. It’s not like they stumbled into this by accident.

[real-or-random]

Concept ACK, great to see a PR for this!

[real-or-random]

nit: I think this one should also be an enable-style configure option instead of a with-style configure options; with is for compiling with external packages e.g., with-libxyz. And what the user provides here is not a package.

[hebasto]

Concept ACK.

[furszy]

Update pushed, thanks for the deep review real_or_random!
I haven’t fully finished addressing all the comments yet, but I'll hopefully finish tomorrow. So many good topics.

[sipa]

A few high-level comments:

• I don't think we should do link-time plugging in the same PR as runtime plugging, and when we do, I don't think it should be done through the "module" abstractions. We use modules for exporting functionality from the library, not for offering replacement of internal dependencies (I find it strange to have base functionality depend on a module!).
• "round" is a term of art in cipher design, and SHA-256 has exactly 64 rounds. Using the same term with another meaning is confusing.
• I would try hard to avoid storing the transformation function pointer inside the secp256k1_sha256 struct. Reviewing many lines that just add an extra parameter being pass through is not hard.

[furszy]

Per feedback, this is how it would look if we pass the function everywhere furszy@8149fc1 vs how it looks when the function lives in the sha256 struct (an updated version of what we currently have here): furszy@eb57082

Let me know what you guys think.

[sipa]

@furszy From a quick glance, the passing of the function everywhere is fine I think.

I'd suggest passing a pointer to secp256k1_hash_context to the sha256 functions rather than the function pointer itself. That makes it easier to (if ever) add additional fields to that context.

[furszy]

Updated per feedback. We now pass the hash context to all functions that need it, without storing any state inside the sha256 struct. The API arg "rounds" has also been renamed to "blocks" to prevent confusion with existing terms. Also, the commit introducing the compile-time feature has been removed for inclusion at a later stage (see #1777 (comment) for more details).

[real-or-random]

Approach ACK

[furszy]

Updated per feedback. Thanks @real-or-random!

[real-or-random]

two nits on the last commit (I like that one!)

[real-or-random]

What is the point of this test? The baseline run makes sense, of course, but it already exists on master.

For the re-run, I'm not entirely sure what it is supposed to test.

1. The first thing it tests is that test_ecdh_generator_basepoint_impl still passes when an external SHA256 compression function is set. But that's not very surprising given that the "external" function simply delegates to the internal one. I don't think that this is a very meaningful test.
2. The other thing it tests is that the custom function was actually called. This is not entirely a meaningful test because the test function itself test_ecdh_generator_basepoint_impl has secp256k1_sha256_write and secp256k1_finalize calls. So we're testing the test function here and not secp256k1_ecdh.

I suspect you had the second item in mind, but simply overlooked the internal sha256 calls?

---

Anyway, I believe a test that this PR should add is:

1. If an external compression function is set, then secp256k1_ecdh(..., NULL, ...) calls the external compression.
   We could also add this:
2. If the external compression function is removed (set to NULL) afterwards, then secp256k1_ecdh(..., NULL, ...) won't call the external compression again. Not sure if it adds much.

And I suspect that's enough in the case of ECDH. Another new thing is the branch on hashfp == NULL and we want to make sure to test both sides of it. The desired test 1 I described above exercises the "then" branch. And the "else" branch should already be covered by existing tests (and hashfp function won't have access to a ctx, so whether it uses an external or internal compression is not a meaningful question).

And I guess what I said here will apply more or less to the tests in the other modules as well.

edit: I still have to look at your most recent updates, but let me say that this one here is my last comment, so everything else is ACK and we're almost ready to go from my side.

[furszy]

Done. Ended up skipping the SHA256 output sanity check during setup so that we can modify the compression function and assert that the results from the default compression versus the ctx-provided one are different.

I know I could have simplified DEFINE_SHA256_TRANSFORM_PROBE even more by not calling the internal function at all and always returning a random or fixed value, but I think behaving like a real one-way compression function is better for testing.

[furszy]

Updated per feedback regarding the tests. Thanks, real-or-random!

I also ended up reorganizing the PR slightly differently:

• The first commit introduces the hash context with all the mechanical changes.
• The second commit adds the API to allow supplying the compression function (and new tests).
• And the third commit implements the multi-block compression speedup.

[real-or-random]

CI has some segfaults unfortunately. Here's the relevant snippet from valgrind:

  ==3305== Invalid read of size 4
  ==3305==    at 0x1237F7: secp256k1_read_be32 (util.h:419)
  ==3305==    by 0x1237F7: secp256k1_sha256_transform_impl (hash_impl.h:48)
  ==3305==    by 0x1237F7: secp256k1_sha256_transform (hash_impl.h:128)
  ==3305==    by 0x110EA9: secp256k1_sha256_write (hash_impl.h:160)
  ==3305==    by 0x11CE35: ellswift_xdh_hash_function_prefix_impl (main_impl.h:501)
  ==3305==    by 0x11CE35: secp256k1_ellswift_xdh (main_impl.h:587)
  ==3305==    by 0x1222D6: ellswift_xdh_ctx_sha256_tests (tests_impl.h:450)
  ==3305==    by 0x122ECE: run_sequential (unit_test.c:275)
  ==3305==    by 0x122ECE: tf_run (unit_test.c:464)
  ==3305==    by 0x122ECE: main (???:8016)
  ==3305==  Address 0x0 is not stack'd, malloc'd or (recently) free'd

[furszy]

CI has some segfaults unfortunately. Here's the relevant snippet from valgrind:

  ==3305== Invalid read of size 4
  ==3305==    at 0x1237F7: secp256k1_read_be32 (util.h:419)
  ==3305==    by 0x1237F7: secp256k1_sha256_transform_impl (hash_impl.h:48)
  ==3305==    by 0x1237F7: secp256k1_sha256_transform (hash_impl.h:128)
  ==3305==    by 0x110EA9: secp256k1_sha256_write (hash_impl.h:160)
  ==3305==    by 0x11CE35: ellswift_xdh_hash_function_prefix_impl (main_impl.h:501)
  ==3305==    by 0x11CE35: secp256k1_ellswift_xdh (main_impl.h:587)
  ==3305==    by 0x1222D6: ellswift_xdh_ctx_sha256_tests (tests_impl.h:450)
  ==3305==    by 0x122ECE: run_sequential (unit_test.c:275)
  ==3305==    by 0x122ECE: tf_run (unit_test.c:464)
  ==3305==    by 0x122ECE: main (???:8016)
  ==3305==  Address 0x0 is not stack'd, malloc'd or (recently) free'd

yep, was working on it. Nothing serious. See #1806.
Will push the small fix here shortly.

[furszy]

CI green again. Ready to go.