tests_exhaustive: check the result of secp256k1_ecdsa_sign #960

pull niooss-ledger wants to merge 1 commits into bitcoin-core:master from niooss-ledger:check-secp256k1_ecdsa_sign-in-tests changing 1 files +3 −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?

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

real-or-random approved
real-or-random commented at 10:18 PM on July 2, 2021: contributor

utACK a1ee83c6546c65d8f5b32acc4a0e1740858ee7d6
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.
sipa commented at 5:26 AM on July 3, 2021: contributor

ACK a1ee83c6546c65d8f5b32acc4a0e1740858ee7d6
real-or-random merged this on Jul 3, 2021
real-or-random closed this on Jul 3, 2021
niooss-ledger deleted the branch on Jul 3, 2021
Fabcien referenced this in commit 5648895350 on Jan 12, 2026
Fabcien referenced this in commit 88c41303f9 on Jan 12, 2026

Contributors

niooss-ledger

real-or-random

sipa

Linked (view graph)

#961 Policy for SECP256K1_WARN_UNUSED_RESULT