Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove SECP256K1_WARN_UNUSED_RESULT from secp256k1_keypair_* functions #1379

Open
w0xlt opened this issue Jul 17, 2023 · 3 comments
Open

Remove SECP256K1_WARN_UNUSED_RESULT from secp256k1_keypair_* functions #1379

w0xlt opened this issue Jul 17, 2023 · 3 comments

Comments

@w0xlt
Copy link

w0xlt commented Jul 17, 2023

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
Copy link
Contributor

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
Copy link
Contributor

sipa commented Jul 17, 2023

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
Copy link
Contributor

real-or-random commented Jul 18, 2023

These functions can actually return 0, but this requires a violation of the API:

int secp256k1_xonly_pubkey_serialize(const secp256k1_context* ctx, unsigned char *output32, const secp256k1_xonly_pubkey *pubkey) {
secp256k1_ge pk;
VERIFY_CHECK(ctx != NULL);
ARG_CHECK(output32 != NULL);
memset(output32, 0, 32);
ARG_CHECK(pubkey != NULL);
if (!secp256k1_xonly_pubkey_load(ctx, &pk, pubkey)) {
return 0;
}

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants