Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

secp256k1_fe_sqrt checks for success #19

Merged
merged 1 commit into from
Jun 2, 2014
Merged

secp256k1_fe_sqrt checks for success #19

merged 1 commit into from
Jun 2, 2014

Conversation

peterdettman
Copy link
Contributor

  • secp256k1_fe_sqrt now checks that the value it calculated is actually a square root.
  • Add return values to secp256k1_fe_sqrt and secp256k1_ge_set_xo.
  • Callers of secp256k1_ge_set_xo can use return value instead of explicit validity checks
  • Add random value tests for secp256k1_fe_sqrt

@peterdettman
secp256k1_fe_sqrt checks for success
09ca4f3 
- secp256k1_fe_sqrt now checks that the value it calculated is actually a square root.
- Add return values to secp256k1_fe_sqrt and secp256k1_ge_set_xo.
- Callers of secp256k1_ge_set_xo can use return value instead of explicit validity checks
- Add random value tests for secp256k1_fe_sqrt
@sipa
Copy link
Contributor

sipa commented May 23, 2014

ACK

@sipa sipa merged commit 09ca4f3 into bitcoin-core:master Jun 2, 2014
sipa added a commit that referenced this pull request Jun 2, 2014
@sipa
Merge pull request #19
7740eff 
09ca4f3 secp256k1_fe_sqrt checks for success (Peter Dettman)
@peterdettman peterdettman deleted the checked-sqrt branch June 22, 2014 09:17
tecnovert pushed a commit to tecnovert/secp256k1 that referenced this pull request Apr 18, 2022
@jonasnick
Merge pull request bitcoin-core#19 from jonasnick/surjectionproof_ini…
809016c 
…t_p_give_up

Add comment to explain effect of max_n_iterations in surjectionproof_…
theStack added a commit to theStack/secp256k1 that referenced this pull request Aug 7, 2023
@theStack
tests: simplify random_fe_non_zero (remove loop limit and unneeded …
37974c5 
…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 theStack mentioned this pull request Aug 7, 2023
Merged
theStack added a commit to theStack/secp256k1 that referenced this pull request Aug 17, 2023
@theStack
tests: simplify random_fe_non_zero (remove loop limit and unneeded …
197430f 
…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
@theStack
tests: simplify random_fe_non_zero (remove loop limit and unneeded …
dc55141 
…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
@real-or-random
Merge #1395: tests: simplify random_fe_non_zero (remove loop limit …
ee7aaf2 
…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
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@peterdettman @sipa