--- Log opened Tue Jan 18 00:00:33 2022
01:30 -!- elsirion_ [~quassel@gateway/tor-sasl/elsirion] has quit [Remote host closed the connection]
01:56 < elichai2> How do people feel about changing `secp256k1_context_randomize` to return void instead of an int? (It's hard to think of ways where it can "fail", especially  now that every context has ecmult)
06:04 < roconnor> is making API changes like that allowed in C?
06:05 < halosghost> roconnor: what do you mean?
06:06 < roconnor> idk, people mixing up old headers with new libraries or vice versa.
06:06 < halosghost> headers and libraries (on any coherent system) should be in lock-step
06:07 < halosghost> however, that would be an API-breaking change
06:07 < sipa> That would obviously be an incompatible change.
06:07 < halosghost> which, for example, would require a library version bump
06:07 < halosghost> (e.g., a soname bump)
06:07 < roconnor> idk if people normally rename the function when doing things like that.
06:07 < halosghost> roconnor: usually not
06:08 < halosghost> elichai2: there are several places where secp256k1 has functions which effectively cannot fail; they tend to unconditionally return 1
06:08 < roconnor> oh, is it because libsecp256k1 isn't compiled as a shared library?
06:08 < sipa> semver allows this in a new major version number (or always within version 0)
06:09 < halosghost> roconnor: there's nothing stopping you from building libsecp as a shared library
06:09 < halosghost> (in fact, I do that for my own prototyping purposes)
06:09 < halosghost> roconnor: soname bumps are how you deal with sign-posting API breakage in a shared library
06:09 < roconnor> but if someone uploads a new shared library, won't older software compiled to the older headers still try to load the new shared library?
06:10 < roconnor> oh
06:10 < sipa> both semver and libtool have mechanisms for supporting breaking API changes
06:10 < sipa> whether a change like this warrants using those is another matter
06:10 < roconnor> what does libtool do?
06:11 < sipa> tooling around building libraries, on various platforms
06:11 < sipa> libsecp's build system uses it
06:11 < roconnor> sorry I mean what does libtool do about API breaking changes?
06:11 < elichai2> halosghost: like what? There are functions that always return 1 if the inputs are valid (can return 0 via the arg_check macro) but I don't remember any ones that unconditionally return 1
06:12 < sipa> @roconnor It depends on the platform, but e.g. on linux it will result in version numbers in the .so files
06:12 < sipa> so that a dynamically linked file won't try to link at runtime against an incompatible one
06:18 < halosghost> elichai2: mmm, I may well have been thinking of those
06:19 < halosghost> roconnor: what sipa just mentioned is a soname bump ☺
06:24 < halosghost> (libtool is an autotools component that automates a lot of the processes around sensible maintenance of libraries)
06:25 < roconnor> sipa: https://github.com/bitcoin-core/secp256k1/commit/b1483f874c6111b7d727d6c6cb3df35c16a880bc#diff-1f71a59e139573dfb0695be308c69455971378cd67ad7946a3703cb725b589fdR51
06:25 < roconnor> I'm presuming the original reason that these became macros instead of inline function is that function name parameter.
06:26 < roconnor> Does that sound right to you?
06:29 < halosghost> elichai2: secp256k1_ec_pubkey_serialize() is documented as “Returns: 1 always”
06:30 < halosghost> 🤷 ☺
06:31 < halosghost> (and, for example, unlike many/most functions exposed by libsecp, ec_pubkey_serialize() does not enable warnings on the caller ignoring the return value)
06:32 < sipa> It can still return 0 if used incorrectly, I think.
06:32 < halosghost> that seems like improper documentation if so
06:33 < sipa> But that's additional sanity checking, and if you disable the normally fatal error callback in your context.
06:33 < sipa> It's not part of the API
06:35 < roconnor> we could add an ARG_CHECK(seed) to secp256k1_context_randomize :P
06:36 < sipa> seed = NULL is valid (it resets the randomization to a deterministic state)
06:37 < halosghost> sipa: when you said “it can return 0 if used incorrectly”, were you talking about _ec_pubkey_serialize(), or about _context_randomize()?
06:38 < sipa> I meant pubkey_serialize, but I can't check to make sure right now.
06:38 < halosghost> no worries
06:38 < halosghost> (just wanted to make sure I was following)
06:40 < halosghost> it does look like, with invalid arguments (e.g., null context), ec_pubkey_serialize() can indeed return non-1
06:41 < sipa> Right, but that means you're not following the documented contract, so the function isn't required to follow the contract either.
06:42 < sipa> It's just belt and suspenders.
06:42 < halosghost> sure
06:43 < halosghost> on-first-blush, an implicit “as long as you're calling the function according to the specified API” isn't unreasonable
06:43 < sipa> Yeah, we could add something like that to the top of the header.
06:44 < halosghost> 🤷
06:44 < halosghost> the function lists its requirements in the standard fashion with the rest of the API (ARG_NONNULL, etc.)
06:45 < halosghost> I don't love the use of “always” when that's not true, but I don't think it's unreasonable
06:46 < sipa> In the contract it is true.
06:46 < halosghost> sipa: put it this way, I'm not rushing to my keyboard to open a PR ☺
06:46 < sipa> Sure.
06:48 < halosghost> sipa: I suppose the more practical version of elichai2's question is: if a function truly cannot fail (barring hardware failure, or something else that would be impossible for a caller to recover from), would it be better to have it be void-returning, or to unconditionally return 1?
06:48 < halosghost> s/practical/general/
06:49 < elichai2> halosghost: for example, context_destroy returns void
06:50 < real_or_random> fwiw, I think both are fine. "void" is clearer for the caller. "int" is easier for us to change later (maybe the function could change in future versions)  
06:50 < real_or_random> but I don't think it's worth to break the API for such a detail.
06:51 < real_or_random> of course, if there's an API overhaul, one can think about these details 
06:51 < sipa> void would affectively prevent us from doing the ARG_CHECK checks (except by always-aborting, which some people seem to really hate)
07:00 < roconnor> halosghost: minor nit regarding ec_pubkey_serialize, VERIFY_CHECK is applied to ctx, and ctx being null doesn't return 0.  only the ARG_CHECKs do this.
07:02 < halosghost> roconnor: mm, you're right. Am I understanding correctly that VERIFY_CHECK() will abort() if the condition fails?
07:03 < roconnor> depending on the compile flags, VERIFY_CHECKs will be ignored.
07:04 < roconnor> I mean, executed, and the result ignored.
07:06 < real_or_random> roconnor: at the moment, it's not even executed I think
07:07 < real_or_random> there's an open PR that tries to (ab)use compiler optimization to prove that VERIFY_CHECK don't have side effects   
07:08 < halosghost> hehe
07:12 < halosghost> roconnor: regardless, because it does ARG_CHECKs, those can result in 0-return; right?
07:13 < halosghost> (in addition to the error callback)
07:16 < roconnor> Yes it is the ARG_CHECKs that cause return 0 to exist.
07:17 < halosghost> 👍
--- Log closed Wed Jan 19 00:00:34 2022