Use `SECP256K1_WARN_UNUSED_RESULT` internally #1872

Maybe we should use SECP256K1_WARN_UNUSED_RESULT also internally. Currently we only have this in public API functions.

Originally posted by @real-or-random in #1867#pullrequestreview-4466893763

I think #1876 somewhat misguided because it adds the warning to almost every function, but it exposes an interesting observation: We have some internal functions which always return 1, e.g., secp256k1_load_pubkey. These should probably return void instead.

(For API functions, we tend to prefer returning an int, even if it's always 1. I'm not entire sure what the reason is. My best guess is that the idea is that these can be changed to return 0 in some cases without breaking the API. But if that's the reason, then it's a bit questionable because it's still a "semantic" break of the API: Callers who didn't check the return value of a function in libsecp256k1 0.x because it was documented to be always 1 have a high change of missing that that function can now return 0 in 0.x+1. Anyway, that's not the topic of this issue.)

I think #1876 somewhat misguided because it adds the warning to almost every function, but it exposes an interesting observation: We have some internal functions which always return 1, e.g., secp256k1_load_pubkey. These should probably return void instead.

Sounds reasonable. Looking at {_xonly,}_pubkey_load specifically, most API functions that call it currently check for its return value, but a few also don't (e.g. in ECDH or _pubkey_combine). Not having a return value in the first place would remove the ambiguity and simplify the call-sites a lot. On the other hand, I'm wondering whether this could lead to (indirect) API behavior change. The pubkey loading functions don't directly return 0, but their ARG_CHECKs could. If the default illegal_callback is used, then this point is never reached as we crash (via abort()), and the behavior is the same before and after the change. If the illegal_callback is set by users to not abort (e.g. just print out a message, or doing nothing), then behavior could change, as we'd currently return from the API functions after failed pubkey loading, whereas after such a change we would continue with a malformed _pubkey object, leading maybe to weird behavior later. But maybe that's too far-fetched and just fine, as the user can't expect a reasonable continuing anyway in that case?

(Thinking of the silentpayments module PR, I'd be honestly very glad if I could just remove the {_xonly,}_pubkey_load return value checks; as long as the return value exists that feels wrong though).

(For API functions, we tend to prefer returning an int, even if it's always 1. I'm not entire sure what the reason is. My best guess is that the idea is that these can be changed to return 0 in some cases without breaking the API. But if that's the reason, then it's a bit questionable because it's still a "semantic" break of the API: Callers who didn't check the return value of a function in libsecp256k1 0.x because it was documented to be always 1 have a high change of missing that that function can now return 0 in 0.x+1. Anyway, that's not the topic of this issue.)

Hmm that's a very good point.