search

bitcoin-core / secp256k1

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

#1570 improve examples: remove key generation loop #1599

Open Cheapshot003 opened 1 month ago

Cheapshot003 commented 1 month ago

Hi! As discussed in #1570 I propose this as a change to the examples.c

If the key is either zero or out of range we just return 1 and end the example instead of looping until a good key is found. This is all very unlikely but could indicate a faulty/manipulated RNG. Anybody knows where I can find the docs for this? First commit to this library, hope I don't break anything!

Cheapshot003 commented 1 month ago

Hey @Cheapshot003, thanks for the contribution. What sort of docs are you looking for exactly?

If you look here someone talks about API docs that should be changed. I don't find any docs related to the examples.

jonasnick commented 1 month ago

Ah. I don't know which API docs @real-or-random meant.

Cheapshot003 commented 1 month ago

ok @jonasnick should be all fixed now :)

theStack commented 1 month ago

@Cheapshot003: can you squash your commits please, see e.g. https://github.com/bitcoin/bitcoin/blob/9cb9651d92ddb5d92724f6a52440601c7a0bbcf8/CONTRIBUTING.md?plain=1#L202-L230? You may also want to ensure that the resulting commit has the correct username and e-mail address set, so it will be attributed to you (the last few commits have just the user name "root" :-))

Cheapshot003 commented 1 month ago

Ah I pushed from another machine that wasn't properly configured. Squashed commits, should be alright now. Thx for the help!

real-or-random commented 1 month ago

Ah. I don't know which API docs @real-or-random meant.

My thinking was that the docs of secp256k1_ec_seckey_verify could not only say that the probability of getting an invalid key is negligible, but also spell out the truth that if this occurs on a randomly generated key, then the caller should assume that the randomness source is severely broken and not retry.
https://github.com/bitcoin-core/secp256k1/blob/1988855079fa8161521b86515e77965120fdc734/include/secp256k1.h#L682-L697

Please don't hesitate to add a commit, but we can also do this in a different PR instead.

(Now that I see this, we should also replace "ECDSA secret key" by "elliptic curve secret key" or just "secret key". This dates back to the early days of the library, when it supported only ECDSA.)

Cheapshot003 commented 1 month ago

Oh thx, I'll adjust these when I come home in the afternoon. I think it would make sense to do it within this pull request