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!
64+ if (!fill_random(seckey1, sizeof(seckey1)) || !fill_random(seckey2, sizeof(seckey2))) {
65+ printf("Failed to generate randomness\n");
66+ return 1;
67+ }
68+ if (!secp256k1_ec_seckey_verify(ctx, seckey1) || !secp256k1_ec_seckey_verify(ctx, seckey2)) {
69+ printf("Generated secret key is invalid. This could indicate an issue with the random number generator.\n")
seckey_verify fails. Also, “could indicate” -> “indicates”.
58- }
59- if (secp256k1_ec_seckey_verify(ctx, seckey1) && secp256k1_ec_seckey_verify(ctx, seckey2)) {
60- break;
61- }
62+ * order), we return 1. Note that the probability of this
63+ * happening is negligible, though it could indicate a faulty RNG */
0/* If the secret key is zero or out of range (greater than secp256k1's
1 * order), we return 1. Note that the probability of this occurring
2 * is negligible with a properly functioning random number generator. */
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.
improving examples #2: fixing my mistakes; changing comments
improving examples #3: changing comments
fixing syntax - adjustment to schnorr examples
fixing comments
fix schnorr signature tests
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.)
59- if (secp256k1_ec_seckey_verify(ctx, seckey1) && secp256k1_ec_seckey_verify(ctx, seckey2)) {
60- break;
61- }
62+/* If the secret key is zero or out of range (greater than secp256k1's
63+* order), we return 1. Note that the probability of this occurring
64+* is negligible with a properly functioning random number generator. */
here and in examples/schnorr.c, the comment should be at the same indentation level as the code
0 /* If the secret key is zero or out of range (greater than secp256k1's
1 * order), we return 1. Note that the probability of this occurring
2 * is negligible with a properly functioning random number generator. */
Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
61+ printf("Failed to generate randomness\n");
62+ return 1;
63+ }
64+ if (!secp256k1_ec_seckey_verify(ctx, seckey1) || !secp256k1_ec_seckey_verify(ctx, seckey2)) {
65+ printf("Generated secret key is invalid. This indicates an issue with the random number generator.\n");
66+ return 1;
Two final minor remarks from my side (applies to all the example files):
if because it applies to that one.
Looks good from my side except minor corrections.
Can you squash your commits again after addressing the comments? I recommend either keeping it two logical commits (one for the examples and one for the docs), or just squashing everything in a single commit if that’s easier. (It’s a bit a matter of taste in the end. The example changes and the doc changes are not exactly of the same nature, but certainly closely related.)