✔️ DONE Getter secp256k1_get_hash_context (#1911)
The long getter name still looks a bit weird to me. It makes it hard for the eye to parse lines such as this:
secp256k1_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:
secp256k1_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.
TODO Reorganize 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)
TODO Potentially improve the smoke test
See #1905
✔️ DONE Document, that, if the user calls some built-in nonce/ecdh-hash/... function manually, then it won't use the SHA256 override (4147f8bdf628fa95b42adc1d7ce55436eb63861e)
Or do something else, e.g., come up with a better API
From IRC today:
08: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
08:12 < real_or_random> e.g., nonce derivation or hashing the ECDH raw secret
08:12 < sipa> right, only the internal variants get to make use of the context-dependent SHA override
08:13 < real_or_random> noticed that in [#1806 (comment)](/bitcoin-core-secp256k1/1806/#issuecomment-4022837213)
08: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
08: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")
08:14 < sipa> i don't think we can retrofit this into the current API, it needs new functions
08: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
08:15 < sipa> i think that's what the code does?
08:16 < real_or_random> you're saying we check for NULL?
08:16 < real_or_random> i.e., if the function ptr is NULL
08:16 < sipa> https://github.com/bitcoin-core/secp256k1/blob/master/src/modules/ellswift/main_impl.h#L565L572
08:17 < sipa> are you talking about something else?
08:17 < real_or_random> no, I think I was simply mistaken
08:18 < real_or_random> the one in ellswift is a bit different. it doesn't have a "NULL" default value
08:18 < real_or_random> but yes, we compare function pointers
08: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
08: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
08:19 < real_or_random> yes, indeed.
08: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.
08:21 < sipa> yup
08: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
08:23 < real_or_random> maybe it should be noted somewhere in the docs that the override isn't always active
08: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
08:25 < real_or_random> and secp256k1_ellswift_xdh could remain an advanced API function that takes a function pointer
08:25 < sipa> hmm, yes, indeed
08:26 < real_or_random> this would avoid having two generations of user-provided functions (or the breakage that comes with them)
08:26 < real_or_random> but still not sure if it's worth the hassle
08:26 < sipa> indeed, i think what we have is good enough in practice
08:26 < real_or_random> we have a similar thing already in the schnorrsig module, where we have a simple and an advanced signing function
08:27 < sipa> it doesn't scale well if we had tons of built-in functions we'd want to optimize
08:27 < real_or_random> yeah but I believe 99% of users call it with the default
08:27 < sipa> indeed, or one of the defaults
08:28 < real_or_random> and the other 1% is people who want the raw ECDH secret and they don't need SHA256 anyway
08: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
TODO musig also uses hash_ctx but has no equivalent ctx_sha256 test
Found by @mllwchrry (https://github.com/BlockstreamResearch/secp256k1-zkp/pull/365#issuecomment-4614467625)
More context from @furszy:
There are three API functions that could/should have a custom compression test secp256k1_musig_pubkey_agg, secp256k1_musig_nonce_process and secp256k1_musig_nonce_gen.
If my memory doesn't fail me, the last one is slightly special because the session secret randomness is cleared internally, which will affect the result for the comparison check. But it is just a matter of caching the value and re-seeding it.
