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

use EXIT_ codes instead of magic numbers for exit(...) and main return values #1609

Open
theStack opened this issue Sep 27, 2024 · 3 comments

Comments

@theStack
Copy link
Contributor

This is really only a minor issue, but I noticed while reviewing #1479 that the return codes of functions in the examples could potentially be confusing. Throughout the API and internal functions we use 0=failure/1=success, while for the main function and (exit(...)) it's the other way round, i.e. 0=success/1=failure. We could use EXIT_{SUCCESS,FAILURE} (defined in stdlib.h, see https://en.cppreference.com/w/c/program/EXIT_status) for the latter instead for more clarity.

See e.g. bitcoin/bitcoin@4441018 for a comparable change in Bitcoin Core as orientation. This could be a good first issue.

@real-or-random
Copy link
Contributor

real-or-random commented Sep 27, 2024

Concept ACK

I had the same thought in the past.

My suggestion to remove assert() in the examples is slightly related and could be addressed in another commit in the same PR that would resolve this issue here.

@Cheapshot003
Copy link
Contributor

While working on this I noticed a mistake(?) in the comments in the verification section of ecdsa.c line 95 and 101 (probably other examples too).

https://github.com/Cheapshot003/secp256k1/blob/01b5893389efe6615d9ec2ee5f7f0f46c44b1c41/examples/ecdsa.c#L92C1-L102C6

The comment says "This will return 0 if the signature can't be parsed correctly" however the return value in case of failure is 1. Shall I just adjust it together with the other return values and replace it with EXIT_FAILURE?

@real-or-random
Copy link
Contributor

real-or-random commented Oct 14, 2024

The comment says "This will return 0 if the signature can't be parsed correctly" however the return value in case of failure is 1. Shall I just adjust it together with the other return values and replace it with EXIT_FAILURE?

I think what is meant here is that secp256k1_ecdsa_signature_parse_compact (and the other called function) will return 0 in case of failure. (And yes, then the main function in the example will return 1, which is precisely demonstrates the confusion described in this issue.)

But yeah, the fact that you misread this is proof that this is misleading and should be improved.

Shall I just adjust it together with the other return values and replace it with EXIT_FAILURE?

That sounds reasonable, yes. At the risk of stating the obvious, it makes sense to have multiple independent but related commits under the same theme in a single PR, at least if they're small. And in this case, the overarching theme of the PR would be "Clean up examples and make them clearer". An exception can be warranted if some of the commits are more important/more urgent, and we want to make sure that these get in and are not held up by lack of review or required changes in the less important commits.

Cheapshot003 added a commit to Cheapshot003/secp256k1 that referenced this issue Oct 15, 2024
theStack pushed a commit to theStack/secp256k1 that referenced this issue Feb 14, 2025
theStack added a commit to theStack/secp256k1 that referenced this issue Feb 14, 2025
theStack added a commit to theStack/secp256k1 that referenced this issue Feb 14, 2025
real-or-random added a commit that referenced this issue Feb 24, 2025
…program execution status

13d3896 CONTRIBUTING: mention that `EXIT_` codes should be used (Sebastian Falbesoner)
c855581 test, bench, precompute_ecmult: use `EXIT_...` constants for `main` return values (Sebastian Falbesoner)
965393f examples: use `EXIT_...` constants for `main` return values (Sebastian Falbesoner)

Pull request description:

  This simple PR addresses #1609 for all example and internal binaries. Alternative to #1618, which is stale (the author confirmed to me that they are not working on that PR anymore). The last commits adds a suggestion to CONTRIBUTING.md, not sure though if we want to go that far.

ACKs for top commit:
  jonasnick:
    ACK 13d3896
  real-or-random:
    utACK 13d3896

Tree-SHA512: 513eba4b712ba3d5f23a5fdc51cb27c5347b29bcaba39501345913c220be6f093a41186911032d2ddc898b848de84f05f374b3554ffcf92610728b2a23c0bb36
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