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.

Following up on the discussion from #1876, where it was suggested that functions like secp256k1_pubkey_load and secp256k1_xonly_pubkey_load should return void (since they always return 1), I audited the ~40 call sites across the different modules (musig, silentpayments, ellswift, etc.) and removed all their return-value checks.

However, I ran into the exact design question that @theStack pointed out here. Because ARG_CHECK_VOID just returns without a value on failure, if a user overrides the illegal callback to be non-aborting, the code just continues executing with an uninitialized garbage public key.

In fact, when I ran the test suite with my changes, 24 tests failed (like secp256k1_silentpayments_sender_create_outputs) because the test suite itself relies on a non-aborting callback. The tests expect the API to return 0 when given an invalid pubkey, but with the void changes, the functions fell through and returned 1 (success) with garbage data.

How would you prefer this be handled? Should we:

1. Keep the int return type and just add SECP256K1_WARN_UNUSED_RESULT to the internal functions?
2. Accept that non-aborting callbacks will result in garbage data, and change the test suite to not expect a 0 return on invalid inputs?

Happy to open a PR for either approach once we decide!