search

bitcoin-core / secp256k1

Optimized C library for EC operations on curve secp256k1
MIT License
2.05k stars 991 forks source link

Remove `SECP256K1_WARN_UNUSED_RESULT` from `secp256k1_keypair_*` functions #1379

Open w0xlt opened 1 year ago

w0xlt commented 1 year ago

Why do secp256k1_keypair_* functions have SECP256K1_WARN_UNUSED_RESULT if they always return 1 ? The only exception is secp256k1_keypair_create.

real-or-random commented 1 year ago

Why do secp256k1_keypair_* functions have SECP256K1_WARN_UNUSED_RESULT if they always return 1 ?

Because they are not guaranteed to always return 1 in future (but backwards-compatible) API versions, I suppose.

sipa commented 1 year ago

I don't think that's a concern. If a backward-compatible API change causes 0 to become a permitted return value, it shouldn't occur for code written against the old header file (e.g. it'd require API calls, or constants, or magics, ... that don't exist in the old header). Thus, to any user of the old header file, no warning should be needed, and the WARN_UNUSED_RESULT can just be added to the header whenever such an API change is introduced.

TL;DR: I think we should drop SECP256K1_WARN_UNUSED_RESULT for functions that are defined to only return 1.

real-or-random commented 1 year ago

These functions can actually return 0, but this requires a violation of the API: https://github.com/bitcoin-core/secp256k1/blob/b40e2d30b7a6f71e7d82340c1cf82150c5cf0540/src/modules/extrakeys/main_impl.h#L44-L54

Wouldn't it make more sense to call the illegal callback in that case?