Follow-ups to #1777

real-or-random commented at 3:48 pm on March 9, 2026: contributor

Getter `secp256k1_get_hash_context`

The long getter name still looks a bit weird to me. It makes it hard for the eye to parse lines such as this:

0secp256k1_sha256_write(secp256k1_get_hash_context(ctx), &hash, seckey32, 32);

I admit I contributed to this by suggesting that the hash context object should go first. After reconsidering this, I prefer what we do for the ecmult_gen context:

0secp256k1_ecmult_gen(&ctx->ecmult_gen_ctx, &nonce_ptj[i], &k[i]);

In fact, there are a few calls that just read ctx instead of &ctx->ecmult_gen_ctx. These are certainly code smells because they rely on the order of members in the struct….

In a potential follow-up PR, we could make the way of getting the subcontext consistent across ecmult_gen and hash consistent and also solve these smells. As said above, I’d prefer &ctx->... which is short, and direct accesses to field members are not uncommon in C. The setters still make sense because they validate the args. I don’t insist on that solution, but I think we should at least make it consistent across modules.

An entirely different option will be to pass just ctx and do the unfolding only inside the hash and ecmult_gen modules. But perhaps this makes testing and other stuff harder: now you’ll need a full context to call these functions.

Self-test

I’m not entirely happy with what this PR does to the selftest module. (Again, the current code may have been based on my previous feedback – sorry, if that’s the case.)

At first glance, it feels a bit weird that the API call secp256k1_selftest now uses the built-in implementation even if the caller has set an override. This may be a questionable scenario anyway (secp256k1_selftest is there only for the static context), but nothing in the API says you can’t call it with a proper context. And if you think about it, the behavior in this PR actually makes sense: The docs of secp256k1_selftest say that this function checks for wrong endianness builds, etc., so we probably want to always use the built-in SHA256 implementation in the self-test.

But with that in mind, the function secp256k1_selftest_sha256 should live in the hash module (perhaps with the name secp256k1_sha256_selftest, and the selftest module should call it deliberately (i.e., with an explanatory comment) with the built-in hash implementation.

edit: This may also be an opportunity to split selftest module in selftest.h and selftest_impl.h like every other module.

edit edit: Or get rid of it if only one function remains left in there…

Originally posted by @real-or-random in #1777 (comment)

real-or-random commented at 3:53 pm on March 9, 2026: contributor

Document, that, if the user calls some built-in nonce/ecdh-hash/… function manually, then it won’t use the SHA256 override

Or do something else, e.g., come up with a better API

From IRC today:

 008:12 < real_or_random> one thing I wasn't fully aware of when introducing the SHA runtime override is that it doesn't work where the user can provide a function
 108:12 < real_or_random> e.g., nonce derivation or hashing the ECDH raw secret
 208:12 < sipa> right, only the internal variants get to make use of the context-dependent SHA override
 308:13 < real_or_random> noticed that in [#1806 (comment)](/bitcoin-core-secp256k1/1806/#issuecomment-4022837213)
 408:13 < real_or_random> I'm not sure if there could be a better API design. but it's not trivial, also if we want to keep it backwards compatible
 508:14 < sipa> does that mean we want to introduce a new generation of these functions, which take a new function pointer type, which receives the sha update pointer (or even just the context, and we expose a "do hash updating with this context")
 608:14 < sipa> i don't think we can retrofit this into the current API, it needs new functions
 708:15 < real_or_random> sipa: at the moment, even the built-in variants don't get the override. we can't really check if the user happens to provide a pointer to a built-in function
 808:15 < sipa> i think that's what the code does?
 908:16 < real_or_random> you're saying we check for  NULL?
1008:16 < real_or_random> i.e., if the function ptr is NULL
1108:16 < sipa> https://github.com/bitcoin-core/secp256k1/blob/master/src/modules/ellswift/main_impl.h#L565L572
1208:17 < sipa> are you talking about something else?
1308:17 < real_or_random> no, I think I was simply mistaken
1408:18 < real_or_random> the one in ellswift is a bit different. it doesn't have a "NULL" default value  
1508:18 < real_or_random> but yes, we compare function pointers
1608:18 < sipa> it's effectively implementing a translation table, which for a few well-defined common functions actually invokes an internal different one that is context-aware
1708:19 < real_or_random> in the other instances we also compare to NULL https://github.com/bitcoin-core/secp256k1/blob/1aafe15139976b0142d791aaf4963de3fc1ff736/src/modules/schnorrsig/main_impl.h#L152-L157
1808:19 < real_or_random> yes, indeed.
1908:21 < real_or_random> I think that's pragmatic given the current API, but it feels a bit awkward. if you pass, for whatever reason, a simple wrapper instead of the original function pointer, you suddenly get different performance. and different illegal/error callbacks.   
2008:21 < sipa> yup
2108:22 < real_or_random> I don't know. I'll think a bit more about it, but my feeling is that it's probably good enough now that we have this API
2208:23 < real_or_random> maybe it should be noted somewhere in the docs that the override isn't always active
2308:24 < real_or_random> I believe a simpler API would just have a different API function for each built-in way of hashing, e.g., have secp256k1_ellswift_xdh_bip324 and secp256k1_ellswift_xdh_prefix
2408:25 < real_or_random> and secp256k1_ellswift_xdh could remain an advanced API function that takes a function pointer  
2508:25 < sipa> hmm, yes, indeed
2608:26 < real_or_random> this would avoid having two generations of user-provided functions (or the breakage that comes with them) 
2708:26 < real_or_random> but still not sure if it's worth the hassle 
2808:26 < sipa> indeed, i think what we have is good enough in practice
2908:26 < real_or_random> we have a similar thing already in the schnorrsig module, where we have a simple and an advanced signing function
3008:27 < sipa> it doesn't scale well if we had tons of built-in functions we'd want to optimize
3108:27 < real_or_random> yeah but I believe 99% of users call it with the default
3208:27 < sipa> indeed, or one of the defaults
3308:28 < real_or_random> and the other 1% is people who want the raw ECDH secret and they don't need SHA256 anyway
3408:29 < real_or_random> may still be nice to document the status quo. I think I'll open a "follow-ups" issue that also mentions the other two things I brought up in [#1777](/bitcoin-core-secp256k1/1777/), so we can keep track of it

real-or-random added the label tweak/refactor on Mar 9, 2026

real-or-random added the label user-documentation on Mar 9, 2026

real-or-random commented at 3:58 pm on March 9, 2026: contributor

cc @furszy

theStack commented at 6:34 pm on March 13, 2026: contributor

I admit I contributed to this by suggesting that the hash context object should go first. After reconsidering this, I prefer what we do for the ecmult_gen context:
0secp256k1_ecmult_gen(&ctx->ecmult_gen_ctx, &nonce_ptj[i], &k[i]);
In fact, there are a few calls that just read ctx instead of &ctx->ecmult_gen_ctx. These are certainly code smells because they rely on the order of members in the struct….

I assume that you were referring to the following calls (found by $ git grep "ecmult_gen(ctx"):

https://github.com/bitcoin-core/secp256k1/blob/ffc25a2731fd277e056c6f62aa94eb0fb78e031d/src/ecdsa_impl.h#L274 https://github.com/bitcoin-core/secp256k1/blob/ffc25a2731fd277e056c6f62aa94eb0fb78e031d/src/ecmult_gen_impl.h#L328

These seem to be fine as-is, as each ctx is an instance of secp256k1_ecmult_gen_context* already, not the general secp256k1_context* (If indeed the latter was passed somewhere, I guess we would detect that by getting an “incompatible pointer” warning at least?). Maybe a rename to ecmult_gen_ctx would still make sense though for explicitness?

real-or-random commented at 12:19 pm on March 14, 2026: contributor

These seem to be fine as-is, as each ctx is an instance of secp256k1_ecmult_gen_context* already, not the general secp256k1_context* .

Oh, indeed!

(If indeed the latter was passed somewhere, I guess we would detect that by getting an “incompatible pointer” warning at least?)

Not entirely sure if the compiler warns here. IIUC, you can safely treat a pointer to a struct as a pointer to the first element (though it’s probably not good practice).

Maybe a rename to ecmult_gen_ctx would still make sense though for explicitness?

Probably a good idea. When I read ctx, I immediately assumed that it’s a secp256k1_context because that’s what we do everywhere…

Follow-ups to #1777 #1835

Getter secp256k1_get_hash_context

Self-test

Document, that, if the user calls some built-in nonce/ecdh-hash/… function manually, then it won’t use the SHA256 override

Getter `secp256k1_get_hash_context`