theStack
commented at 12:51 AM on August 7, 2023:
contributor
random_fe_non_zero contains a loop iteration limit that ensures that we abort if random_fe ever yielded zero more than ten times in a row. This construct was first introduced in PR #19 (commit 09ca4f32) for random non-square field elements and was later refactored into the non-zero helper in PR #25 (commit 6d6102fe). The copy-over to the exhaustive tests happened recently in PR #1118 (commit 0f864207).
This case seems to be practically irrelevant and I'd argue for keeping things simple and removing it (which was already suggested in #1118 (review)); if there's really a worry that the test's random generator is heavily biased towards certain values or value ranges then there should consequently be checks at other places too (e.g. directly in random_fe for 256-bit values that repeatedly overflow, i.e. >= p).
Also, the _fe_normalize call is not needed and can be removed, as the result of random_fe is already normalized.
real-or-random approved
real-or-random
commented at 1:13 AM on August 7, 2023:
contributor
utACK37974c5e007ba70056dc41e61f6c226075305c4f
real-or-random added the label refactor/smell on Aug 7, 2023
real-or-random
commented at 1:16 AM on August 7, 2023:
contributor
To get rid of the duplication, we could move the ge_equals_* functions to group (I guess it doesn't hurt to have them in the code) and the random_fe to testrand. Or we could just move everything to a new testutil.h.
theStack
commented at 5:37 PM on August 17, 2023:
contributor
To get rid of the duplication, we could move the ge_equals_* functions to group (I guess it doesn't hurt to have them in the code) and the random_fe to testrand. Or we could just move everything to a new testutil.h.
Both approaches sound reasonable to me, but I've decided to go with the second one and added a commit introducing a new testutil.h file. Right now only the duplicate functions (random_fe{,_non_zero and ge_equals_{ge,gej}) are moved, in the future it maybe makes sense to also move others that are similar (gej_xyz_equals_gej, random_fe_non_square etc.).
theStack force-pushed on Aug 17, 2023
tests: simplify `random_fe_non_zero` (remove loop limit and unneeded normalize)
`random_fe_non_zero` contains a loop iteration limit that ensures that
we abort if `random_fe` ever yielded zero more than ten times in a row.
This construct was first introduced in PR #19 (commit 09ca4f32) for
random non-square field elements and was later refactored into the
non-zero helper in PR #25 (commit 6d6102fe). The copy-over to the
exhaustive tests happened recently in PR #1118 (commit 0f864207).
This case seems to be practically irrelevant and I'd argue for keeping
things simple and removing it; if there's really a worry that the test's
random generator is heavily biased towards certain values or value
ranges then there should consequently be checks at other places too
(e.g. directly in `random_fe` for 256-bit values that repeatedly
overflow, i.e. >= p).
Also, the _fe_normalize call is not needed and can be removed, as the
result of `random_fe` is already normalized.
This is a metadata mirror of the GitHub repository
bitcoin-core/secp256k1.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2026-05-19 06:52 UTC
