A few improvements to the sqrt tests #25
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sipa
added a commit
that referenced
this pull request
Jun 3, 2014
6d6102f A few improvements to the sqrt tests (Peter Dettman)
theStack
added a commit
to theStack/secp256k1
that referenced
this pull request
Aug 7, 2023
…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 bitcoin-core#19 (commit 09ca4f3) for random non-square field elements and was later refactored into the non-zero helper in PR bitcoin-core#25 (commit 6d6102f). The copy-over to the exhaustive tests happened recently in PR bitcoin-core#1118 (commit 0f86420). 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.
theStack
added a commit
to theStack/secp256k1
that referenced
this pull request
Aug 17, 2023
…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 bitcoin-core#19 (commit 09ca4f3) for random non-square field elements and was later refactored into the non-zero helper in PR bitcoin-core#25 (commit 6d6102f). The copy-over to the exhaustive tests happened recently in PR bitcoin-core#1118 (commit 0f86420). 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.
theStack
added a commit
to theStack/secp256k1
that referenced
this pull request
Aug 17, 2023
…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 bitcoin-core#19 (commit 09ca4f3) for random non-square field elements and was later refactored into the non-zero helper in PR bitcoin-core#25 (commit 6d6102f). The copy-over to the exhaustive tests happened recently in PR bitcoin-core#1118 (commit 0f86420). 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.
real-or-random
added a commit
that referenced
this pull request
Sep 14, 2023
…and unneeded normalize) c45b7c4 refactor: introduce testutil.h (deduplicate `random_fe_`, `ge_equals_` helpers) (Sebastian Falbesoner) dc55141 tests: simplify `random_fe_non_zero` (remove loop limit and unneeded normalize) (Sebastian Falbesoner) Pull request description: `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 09ca4f3) for random non-square field elements and was later refactored into the non-zero helper in PR #25 (commit 6d6102f). The copy-over to the exhaustive tests happened recently in PR #1118 (commit 0f86420). This case seems to be practically irrelevant and I'd argue for keeping things simple and removing it (which was already suggested in #1118 (comment)); 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. ACKs for top commit: real-or-random: utACK c45b7c4 siv2r: ACK `c45b7c4` (reviewed the changes and tests for both the commits passed locally). Tree-SHA512: 4ffa66dd0b8392d7d0083a71e7b0682ad18f9261fd4ce8548c3059b497d3462db97e16114fded9787661ca447a877a27f5b996bd7d47e6f91c4454079d28a8ac
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.