sha256: smoke-test caller-supplied compression function #1905

real-or-random commented at 12:38 PM on August 3, 2026: contributor

Extended version of #1904. I'd say if we get enough reviews/confidence before the release, let's merge this here. Otherwise, let's stick with the simpler #1904 and leave this here for after the release.

sha256: cross-check caller supplied compression function

The existing selftest hashes one 63 byte string against a known digest.
Which catches a compression function that is wrong everywhere, but not
one that is wrong on multi-block calls, unaligned input, or a state
that is not the IV.

This introduces secp256k1_sha256_smoke_test, which hashes messages of
various lengths and starting offsets with the supplied function,
accumulates the resulting digests with supplied function into a single
one, compares it to the precomputed correct value.

The check runs once during startup, so a faulty compression function
is caught early rather than silently producing incorrect output later.

Measured locally, this takes well below 1ms, which should be negligible
for any application.

See the introduced test for a clear view of the bugs this catches.

Co-authored-by: Tim Ruffing <me@real-or-random.org>

c1c6365d99

real-or-random force-pushed on Aug 3, 2026

real-or-random commented at 12:49 PM on August 3, 2026: contributor

Differences:

This tests multiple alignments of the message, namely aligned to 64 bytes (but not to 128), aligned to 32 bytes (but not to 64), ...., down to no alignment at all. (Since alignment is always a power of 2, this should be equal to @furszy's first proposal in coverage, but more efficient because we need the loop only log(64)+1=7 times, instead of 64 times).
This skips the test if we know that we're dealing with the internal function (as @furszy's initial proposal). This keeps context creation to ~1.5 us on my machine (instead of ~75 us).
A few cleanups suggested by @theStack. (I kept the hash_ctx_init call because it's in principle correct. A hash context may have more stuff to init in the future. Anyway, the compiler will figure out the dead store and optimize it away.)
Renamed the function to smoke_test (equiv isn't a great any longer since it doesn't run the built-in anymore).
Adjust the commit message.

real-or-random added the label assurance on Aug 3, 2026

real-or-random added the label tweak/refactor on Aug 3, 2026

real-or-random requested review from Copilot on Aug 3, 2026

real-or-random renamed this:
~~sha256: cross-check caller supplied compression function~~
sha256: smoke-test caller-supplied compression function
on Aug 3, 2026

?

copilot_work_started real-or-random

in src/hash_impl.h:166 in c1c6365d99

 161 | +
 162 | +    /* Max alignment to test; even AVX512 needs at most 64 byte alignment. */
 163 | +    #define MAX_ALIGNMENT 64 /* local macro, undef'd below */
 164 | +    /* sizeof(msg) = length of longest message
 165 | +     *               + 2 * MAX_ALIGNMENT - 1 for the offset in the initial value of m
 166 | +     *               + 2 * MAX_ALIGNMENT - 1 for the offset in the final value of m */

Copilot commented at 12:56 PM on August 3, 2026:

The comment describing the size calculation refers to sizeof(msg) and "m", but the actual buffer is msg_buf and the pointer being adjusted is msg. This makes the rationale for the stack buffer sizing harder to follow.

in src/hash_impl.h:226 in c1c6365d99

 221 | +    secp256k1_sha256_finalize(&ctx, &sha_accum, out);
 222 | +
 223 | +    return (secp256k1_memcmp_var(accum_expected, out, 32) == 0);
 224 | +    #undef LONGEST
 225 | +    #undef MAX_ALIGNMENT
 226 | +}

Copilot commented at 12:56 PM on August 3, 2026:

#undef LONGEST/#undef MAX_ALIGNMENT are placed after a return. Preprocessor directives still run, but this is easy to misread as dead/unreached cleanup and can lead to confusion about macro leakage; placing the #undefs before the return keeps intent clear.

Copilot commented at 12:56 PM on August 3, 2026: none

Pull request overview

This PR strengthens validation of caller-supplied SHA256 compression callbacks by adding a multi-case “smoke test” that cross-checks behavior across message lengths and input alignments, and integrating it into the existing SHA256 selftest pathway. This helps catch subtle but dangerous misimplementations (e.g., multi-block handling, state handling, or alignment assumptions) early when installing a custom compression function.

Changes:

Add secp256k1_sha256_smoke_test() to validate supplied SHA256 compression callbacks against a precomputed accumulated digest.
Extend secp256k1_selftest_sha256() to run the smoke test for non-default compression functions after the existing known-digest check.
Add unit tests covering several intentionally broken compression callbacks plus a known-good callback wrapper.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
src/tests.c	Adds targeted unit tests exercising the new smoke test against good/bad compression callbacks.
src/selftest.h	Extends SHA256 selftest to run the new smoke test for non-default compression functions.
src/hash_impl.h	Introduces the new SHA256 compression smoke test implementation.
include/secp256k1.h	Documents the new behavior when installing a custom SHA256 compression function.

💡 <a href="/bitcoin-core/secp256k1/new/master?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.

in include/secp256k1.h:438 in c1c6365d99

 430 | @@ -431,6 +431,12 @@ typedef void (*secp256k1_sha256_compression_function)(
 431 |   * noncefp==NULL or noncefp==secp256k1_nonce_function_default is passed
 432 |   * as an argument to secp256k1_ecdsa_sign.)
 433 |   *
 434 | + * Note: The provided function is tested against a set of known SHA256
 435 | + * digests, aborting on any mismatch, in order to catch basic misbehavior
 436 | + * early. Takes well under 1 ms on a desktop machine.
 437 | + * This is NOT a substitute for having proper test coverage of the
 438 | + * supplied function outside this library.

Copilot commented at 12:56 PM on August 3, 2026:

The API doc says the library "aborts on any mismatch", but the actual behavior is to invoke the context's illegal callback (which aborts by default) and then return without installing the function. The current wording overstates the behavior for callers that override the callback.

theStack approved

theStack commented at 2:47 PM on August 3, 2026: contributor

ACK c1c6365d99dea5d6e68f4bd71136424c7e5e5223

Being thorough with alignment checks seems the right approach, even though it adds quite a bit of complexity to the code (it LGTM, but it's very easy to get something wrong there or overlook something as reviewer).

The following Copilot suggestions seem to make sense to tackle:

in src/hash_impl.h:154 in c1c6365d99

 149 | +    /* SHA256 works on 64 byte blocks, secp256k1_sha256_write gives as many blocks
 150 | +     * at once to compression, so the count is what varies here. A SIMD implementation
 151 | +     * typically hashes four or eight at a time, then any left over one by one.
 152 | +     * These lengths cover every number from 1 to 9, which includes counts that
 153 | +     * divide evenly and counts leaving one, two or three over. */
 154 | +    #define LONGEST 576  /* local macro, undef'd below */

theStack commented at 3:03 PM on August 3, 2026:

nit: an alternative here and for MAX_ALIGNMENT below would be

    enum { LONGEST = 576 };

for not having to #undef below, leading to a smaller patch; but not sure if that's strictly better or if we even have a preference in the project (we seem to use both)

real-or-random commented at 7:48 PM on August 3, 2026:

I tend to think that enum is better in this case. I always forget that it's a compile-time constant that can be used in array sizes.

in src/hash_impl.h:189 in c1c6365d99

 184 | +    secp256k1_hash_ctx_init(&ctx);
 185 | +    ctx.fn_sha256_compression = fn_compression;
 186 | +    secp256k1_sha256_initialize(&sha_accum);
 187 | +
 188 | +    /* Make msg a pointer into msg_buf aligned to 2 * MAX_ALIGNMENT boundary. */
 189 | +    offset = ((2 * MAX_ALIGNMENT) - ((uintptr_t)msg_buf % (2 * MAX_ALIGNMENT))) % (2 * MAX_ALIGNMENT);

theStack commented at 3:04 PM on August 3, 2026:

nit-like (?): uintptr_t does not seem to be part of C89; it was introduced in C99 and is optional even there. Not sure how strict we want to be about this level of portability, though, given that we already rely on stdint.h being available.

real-or-random commented at 7:47 PM on August 3, 2026:

Hm, yes. Almost all architectures have an uintptr_t, but some don't.

The reason not to provide one is that there is no integer type large enough to hold a pointer value so that it can be converted back without loss. We don't need to conversion back, but we still can't just cast to any unsigned integer type because C99 says: If the result cannot be represented in the integer type, the behavior is undefined.

We could ignore the problem and simply require uintptr_t. We could wrap the offset calculation into a preprocessor check that checks for the presence of the type (#ifdef UINTPTR_MAX) and fall back if it's not present (e.g., simply set offset = 0, effectively not doing alignment checks) for these rare architectures.

[1] CHERI archs can't provide it it according to https://www.ralfj.de/blog/2022/04/11/provenance-exposed.html. According to https://www.open-std.org/JTC1/SC22/WG14/www/docs/n2873.htm, there are at least two architectures, but they're not mentioned explicitly in the document.

theStack added this to the milestone 0.8.0 on Aug 3, 2026

furszy commented at 4:23 PM on August 3, 2026: member

Simple coverage for the introduced changes:

diff --git a/src/tests.c b/src/tests.c
--- a/src/tests.c
+++ b/src/tests.c
@@ -517,12 +517,28 @@
     s[0] ^= 1;
 }
 
+/* Wrong when input is 64-byte aligned, like a broken SIMD fast path. */
+static void sha256_transform_aligned_fail(uint32_t *s, const unsigned char *chunk, size_t blocks) {
+    int aligned = ((uintptr_t)chunk % 64) == 0;
+    secp256k1_sha256_transform(s, chunk, blocks);
+    if (aligned) s[0] ^= 1;
+}
+
+/* Wrong on any unaligned input. */
+static void sha256_transform_unaligned_fail(uint32_t *s, const unsigned char *chunk, size_t blocks) {
+    int aligned = ((uintptr_t)chunk % 64) == 0;
+    secp256k1_sha256_transform(s, chunk, blocks);
+    if (!aligned) s[0] ^= 1;
+}
+
 static void run_sha256_compression_smoke_test_tests(void) {
     CHECK(secp256k1_sha256_smoke_test(sha256_transform_noadvance) == 0);
     CHECK(secp256k1_sha256_smoke_test(sha256_transform_short) == 0);
     CHECK(secp256k1_sha256_smoke_test(sha256_transform_ivreset) == 0);
     CHECK(secp256k1_sha256_smoke_test(sha256_transform_batch4) == 0);
     CHECK(secp256k1_sha256_smoke_test(sha256_transform_corrupt) == 0);
+    CHECK(secp256k1_sha256_smoke_test(sha256_transform_aligned_fail) == 0);
+    CHECK(secp256k1_sha256_smoke_test(sha256_transform_unaligned_fail) == 0);
     CHECK(secp256k1_sha256_smoke_test(good_sha256_compression) == 1);
 }

theStack referenced this in commit e0683504d2 on Aug 3, 2026

real-or-random marked this as a draft on Aug 3, 2026