Use SHA256 override for pointers to known aux functions #1900

real-or-random commented at 8:43 AM on July 29, 2026: contributor

Resolves one item in #1835.

real-or-random added this to the milestone 0.8.0 on Jul 29, 2026

real-or-random added the label user-documentation on Jul 29, 2026

real-or-random added the label tweak/refactor on Jul 29, 2026

real-or-random commented at 9:13 AM on July 29, 2026: contributor

cc @furszy

in include/secp256k1.h:431 in dfc3e98c8e

 426 | @@ -427,6 +427,13 @@ typedef void (*secp256k1_sha256_compression_function)(
 427 |   * specialized implementation. It is NOT meant for replacing SHA256
 428 |   * with a different hash function.
 429 |   *
 430 | + * Since auxiliary functions exposed by the library via a function
 431 | + * pointer such as secp256k1_nonce_function_default not take a context

theStack commented at 9:22 AM on July 29, 2026:

missing verb

 * pointer such as secp256k1_nonce_function_default don't take a context

real-or-random commented at 11:09 AM on July 29, 2026:

fixed

in include/secp256k1.h:435 in dfc3e98c8e

 426 | @@ -427,6 +427,13 @@ typedef void (*secp256k1_sha256_compression_function)(
 427 |   * specialized implementation. It is NOT meant for replacing SHA256
 428 |   * with a different hash function.
 429 |   *
 430 | + * Since auxiliary functions exposed by the library via a function
 431 | + * pointer such as secp256k1_nonce_function_default not take a context
 432 | + * object, they will not use the callback when called directly from user
 433 | + * code. (But they will use the callback when called from other library
 434 | + * functions that do take a context object, e.g., when
 435 | + * secp256k1_nonce_function_default is passed to secp256k1_ecdsa_sign.)

theStack commented at 10:02 AM on July 29, 2026:

seems that for secp256k1_ecdsa_sign (and also for secp256k1_ecdh), only passing NULL as function pointer currently leads to usage of the context-aware nonce function call, so the recommendation to pass _nonce_function_default explicitly wouldn't work: https://github.com/bitcoin-core/secp256k1/blob/528863e61f55ba3502f680643c55491bca04c150/src/secp256k1.c#L562-L567

(maybe something we could still fix? // EDIT: thinking of something like https://github.com/theStack/secp256k1/commit/6847ed4475cca7602efe950ed9139010b3e0ff22)

real-or-random commented at 10:42 AM on July 29, 2026:

Ah yes, I thought that's the case because we did the same for schnorrsig (as you noticed) and also for ellswift. I will include it in this PR.

so the recommendation to pass _nonce_function_default explicitly

Okay, yes, and I didn't intent to add this recommendation. If users want the default, passing NULL directly is more natural (though it doesn't make a difference in the end). I will try to rephrase this.

real-or-random commented at 11:09 AM on July 29, 2026:

added a commit

real-or-random force-pushed on Jul 29, 2026

real-or-random renamed this:
~~header: Add note on SHA256 override and aux functions~~
Use SHA256 override for pointers to known aux functions
on Jul 29, 2026

real-or-random removed the label tweak/refactor on Jul 29, 2026

real-or-random added the label bug on Jul 29, 2026

real-or-random added the label tweak/refactor on Jul 29, 2026

theStack commented at 11:19 AM on July 29, 2026: contributor

ACK modulo s/245/256/ nit in 2f38d0cb27854d8b7c87dc0b958dfe77ce13c2b1 commit subject

ecdsa/ecdh: Use SHA256 override if known noncefp/hashfp is passed ed091bc49d

header: Add note on SHA256 override and aux functions

Resolves one item in #1835.

4147f8bdf6

real-or-random force-pushed on Jul 29, 2026

real-or-random commented at 11:23 AM on July 29, 2026: contributor

ACK modulo s/245/256/ nit in 2f38d0c commit subject

lol, fixed :)

theStack approved

theStack commented at 11:28 AM on July 29, 2026: contributor

ACK 4147f8bdf628fa95b42adc1d7ce55436eb63861e

real-or-random commented at 2:18 PM on July 29, 2026: contributor

cc @hebasto want to review this too, so it can safely make it into the release?

hebasto approved

hebasto commented at 5:42 PM on July 29, 2026: member

ACK 4147f8bdf628fa95b42adc1d7ce55436eb63861e, I have reviewed the code and it looks OK.

in include/secp256k1.h:435 in 4147f8bdf6

 426 | @@ -427,6 +427,14 @@ typedef void (*secp256k1_sha256_compression_function)(
 427 |   * specialized implementation. It is NOT meant for replacing SHA256
 428 |   * with a different hash function.
 429 |   *
 430 | + * Since auxiliary functions exposed by the library via a function
 431 | + * pointer such as secp256k1_nonce_function_default do not take a
 432 | + * context object, they will not use the callback when called directly
 433 | + * from user code. (But they will use the callback when called from
 434 | + * other library functions that do take a context object, e.g., when
 435 | + * noncefp==NULL or noncefp==secp256k1_nonce_function_default is passed

furszy commented at 7:02 PM on July 29, 2026:

small note; this comment kinda assumes secp256k1_nonce_function_default will always be equal to the used secp256k1_nonce_function_rfc6979 (we are using that instead of the default in the first commit).

As there are no plans to change the default value ever, it is not a problem. Just something that itches me a bit.

theStack commented at 11:04 PM on July 29, 2026:

I had similar thoughts, but also think it's fine. Even if we ever changed the default value (very unlikely indeed), we would then want to also adapt/extend the if clause(s) accordingly to also use the ctx-aware function for that e.g.

const secp256k1_nonce_function secp256k1_nonce_function_default = nonce_function_new;

{
    ...
    if (noncefp == NULL || noncefp == secp256k1_nonce_function_new) {
        ret = nonce_function_new_impl(...);
    } else if (noncefp == secp256k1_nonce_function_rfc6979) {
        ret = nonce_function_rfc6979_impl(...);
    } else {
        ret = !!noncefp(...);
    }
    ...
}

and the comment would still apply.

real-or-random commented at 2:40 PM on July 30, 2026:

If I understand correctly, what @furszy wanted to say here is that the docs would get out-of-date should we ever switch to a default function which does not use SHA256.

Switching the default nonce function to a more efficient one was discussed earlier. It wouldn't be entirely crazy but at this age of the repo, we'd probably rather do this when (if!) we add a modern ECDSA module.
But switchting the default nonce function to one not based on SHA256 would be crazy. This should address @furszy's concern.

theStack commented at 3:19 PM on July 30, 2026:

Oh, I see now, sorry for misinterpreting then if that was the case. Yeah, not using SHA256 seems pretty wild and far-fetched :)

furszy commented at 7:06 PM on July 29, 2026: member

ACK 4147f8bdf628fa95b42adc1d7ce55436eb63861e

theStack merged this on Jul 29, 2026

theStack closed this on Jul 29, 2026