Policy for SECP256K1_WARN_UNUSED_RESULT

[real-or-random]

By the way, I am wondering whether attribute SECP256K1_WARN_UNUSED_RESULT should be added to function secp256k1_ecdsa_sign: as (according to the documentation of this function) the nonce generation function may fail, it seems to be a good idea to force callers to check the value returned by this function. What do you think about this?

The default nonce generation function will fail only with astronomically low probability. So if you know that you have a valid secret key and you use the default nonce function (99% of the use cases), it's okay not to check the return value.

Having said that, I think we're not entirely consistent here... For example, the same argument would apply to secp256k1_ec_seckey_verify (https://github.com/bitcoin-core/secp256k1/blob/master/include/secp256k1.h#L632). Even secp256k1_ec_pubkey_negate
https://github.com/bitcoin-core/secp256k1/blob/master/include/secp256k1.h#L650 has SECP256K1_WARN_UNUSED_RESULT even if it's guaranteed to return 1 according to the docs...

Maybe we should have a look at this in #783 or in a follow up PR.

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

So I wonder what our (unwritten) policy for SECP256K1_WARN_UNUSED_RESULT should be. I think we're overdoing it in the two mentioned cases but I'm happy to hear other opinions.

[real-or-random]

The default nonce generation function will fail only with astronomically low probability

I don't get this. ecdsa_sign will retry the nonce function if it fails. The failure case for ecdsa_sign is when the secret is invalid, e.g. an attacker has convinced you to tweak your key in various ways that result in invalid private keys.

Ok, I was wrong. The correct statement is that the default nonce generation will never fail.
ecdsa_sign fails if "the nonce generation function failed, or the secret key was invalid."

Do you have an opinion about SECP256K1_WARN_UNUSED_RESULT?

[real-or-random]

are there any musig API functions where the SECP256K1_WARN_UNUSED_RESULT attribute should be added?

That's a good point. I couldn't discover a consistent pattern we're currently following (e.g, secp256k1_ec_pubkey_parse has the warning, secp256k1_tagged_sha256 has the warning even though it always returns 1, and signature_parse_der, signature_parse_compact don't). I think this should be made more consistent treewide with a note to CONTRIBUTING.md.

There are different rules we could follow:

1. Just add the warning basically everywhere. Maybe that could be annoying sometimes if the user has data from a trusted source.
2. Add it to functions returning a boolean result that would be pointless to call without checking for the result (e.g., verify).
3. Add to all functions that can fail without calling a callback (e.g. they can fail because they are receiving data from untrusted sources -- at least in normal scenarios).
4. Add it to all functions you need to call before verify to make sure that verify doesn't succeed due to invalid values. However, this would be defense-in-depth because it shouldn't happen anyway.
5. Add it to all functions that operate on a seckey and produce a public output (e.g. sign) and all the functions the user needs to call before. This prevents that an invalid output (e.g., signature) accidentally leaks information about the seckey. However, users should already be protected from this because we zeroize the outputs if these functions fail.
6. Add it to functions which sound particularly scary (nonce generation).

I added it to a few more functions (following basically rules 2, 3, 4, 6), because removing the warning is backwards compatible.

Originally posted by @jonasnick in #1479 (comment)