#1570 improve examples: remove key generation loop #1599
Conversation
There was a problem hiding this comment.
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. |
|
Ah. I don't know which API docs @real-or-random meant. |
|
ok @jonasnick should be all fixed now :) |
|
@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
force-pushed
the
#1570-improve-examples
branch
from
2fc0845
to
3e8170c
Compare
|
Ah I pushed from another machine that wasn't properly configured. |
My thinking was that the docs of Lines 682 to 697 in 1988855 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.) |
|
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 |
There was a problem hiding this comment.
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.)
examples/ecdh.c
Outdated
| /* If the secret key is zero or out of range (greater than secp256k1's | ||
| * order), we return 1. Note that the probability of this occurring | ||
| * is negligible with a properly functioning random number generator. */ | ||
| if (!fill_random(seckey1, sizeof(seckey1)) || !fill_random(seckey2, sizeof(seckey2))) { | ||
| printf("Failed to generate randomness\n"); | ||
| return 1; | ||
| } | ||
| if (!secp256k1_ec_seckey_verify(ctx, seckey1) || !secp256k1_ec_seckey_verify(ctx, seckey2)) { | ||
| printf("Generated secret key is invalid. This indicates an issue with the random number generator.\n"); | ||
| return 1; |
There was a problem hiding this comment.
Two final minor remarks from my side (applies to all the example files):
- I think the code comment should be moved right above the second
ifbecause it applies to that one. - Perhaps change "we return 1" (which is obvious from the code itself and doesn't deserve a comment) to "we fail".
|
friendly ping :) @Cheapshot003 |
Co-Authored by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
Cheapshot003
force-pushed
the
#1570-improve-examples
branch
from
cce986c
to
cd4f84f
Compare
|
Heyo! Back from vacation! The force push looked a bit scary at first but I think I didn't break anything. Should be alright now! |
4f64b9f4cd build: Drop no longer needed `-fvisibility=hidden` compiler option 5d978f1899 ci: Run `tools/symbol-check.py` 258dba9e11 test: Add `tools/symbol-check.py` 3144ba99f3 Introduce `SECP256K1_LOCAL_VAR` macro e2571385e0 Do not export `secp256k1_musig_nonce_gen_internal` e59158b6eb Merge bitcoin-core/secp256k1#1553: cmake: Set top-level target output locations 18f9b967c2 Merge bitcoin-core/secp256k1#1616: examples: do not retry generating seckey randomness in musig 5bab8f6d3c examples: make key generation doc consistent e8908221a4 examples: do not retry generating seckey randomness in musig 70b6be1834 extrakeys: improve doc of keypair_create (don't suggest retry) 01b5893389 Merge bitcoin-core/secp256k1#1599: bitcoin#1570 improve examples: remove key generation loop cd4f84f3ba Improve examples/documentation: remove key generation loops a88aa93506 Merge bitcoin-core/secp256k1#1603: f can never equal -m 3660fe5e2a Merge bitcoin-core/secp256k1#1479: Add module "musig" that implements MuSig2 multi-signatures (BIP 327) 168c92011f build: allow enabling the musig module in cmake f411841a46 Add module "musig" that implements MuSig2 multi-signatures (BIP 327) 0be79660f3 util: add constant-time is_zero_array function c8fbdb1b97 group: add ge_to_bytes_ext and ge_from_bytes_ext ef7ff03407 f can never equal -m c232486d84 Revert "cmake: Set `ENVIRONMENT` property for examples on Windows" 26e4a7c214 cmake: Set top-level target output locations 4c57c7a5a9 Merge bitcoin-core/secp256k1#1554: cmake: Clean up testing code 472faaa8ee Merge bitcoin-core/secp256k1#1604: doc: fix typos in `secp256k1_ecdsa_{recoverable_,}signature` API description 292310fbb2 doc: fix typos in `secp256k1_ecdsa_{recoverable_,}signature` API description 85e224dd97 group: add ge_to_bytes and ge_from_bytes 7c987ec89e cmake: Call `enable_testing()` unconditionally 6aa576515e cmake: Delete `CTest` module git-subtree-dir: src/secp256k1 git-subtree-split: 4f64b9f4cdf130d2c6d6b7bd855a04faf6f88e08
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!