tests_exhaustive: check the result of secp256k1_ecdsa_sign #960

1. 
   niooss-ledger commented at 3:30 pm on July 2, 2021: contributor

   Hello,

   In test_exhaustive_sign, if secp256k1_ecdsa_sign fails, the signature which is then loaded by secp256k1_ecdsa_signature_load is garbage. Exit early with an error when this occurs.

   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?

2. tests_exhaustive: check the result of secp256k1_ecdsa_sign

   If `secp256k1_ecdsa_sign` fails, the signature which is then loaded by
   `secp256k1_ecdsa_signature_load` is garbage. Exit early with an error
   when this occurs.

   a1ee83c654
3. real-or-random approved
4. 
   real-or-random commented at 10:18 pm on July 2, 2021: contributor

   utACK a1ee83c6546c65d8f5b32acc4a0e1740858ee7d6
5. 
   real-or-random commented at 10:28 pm on July 2, 2021: contributor

   Thanks for the PR!

   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.

6. 
   sipa commented at 5:26 am on July 3, 2021: contributor

   ACK a1ee83c6546c65d8f5b32acc4a0e1740858ee7d6
7. real-or-random merged this on Jul 3, 2021
8. real-or-random closed this on Jul 3, 2021

   ---

9. real-or-random cross-referenced this on Jul 3, 2021 from issue Policy for SECP256K1_WARN_UNUSED_RESULT by real-or-random
10. niooss-ledger deleted the branch on Jul 3, 2021
11. apoelstra cross-referenced this on Jul 27, 2021 from issue Upstream PRs 879, 959, 955, 944, 951, 960, 844, 963, 965 by apoelstra

Contributors
niooss-ledger real-or-random sipa