tests: introduce helper for non-zero `random_fe_test()` results #1358

pull theStack wants to merge 2 commits into bitcoin-core:master from theStack:random_fe_test-refactors changing 1 files +24 −44
  1. theStack commented at 1:06 AM on June 26, 2023: contributor

    There are several instances in the tests where random non-zero field elements are generated by calling random_fe_test in a do/while-loop with is-zero condition. This PR deduplicates all these by introducing a random_fe_non_zero_test helper. Note that some instances checked the is-zero condition via secp256k1_fe_normalizes_to_zero_var, which is unnecessary, as the result of random_field_element_test is already normalized (so strictly speaking, this is not a pure refactor, and there could be tiny run-time improvements, though I doubt that's measurable).

    Additionally, the first commit removes the function random_field_element_test as it is logically a duplicate of random_fe_test.

  2. theStack commented at 1:21 AM on June 26, 2023: contributor

    I guess an alternative would be to not care about the zero-case at all (in the sense of not looping, but only asserting/verify-checking) due to the incredibly low probability of that ever happening? (I just remembered the discussion at https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1171972333, though that was about invalid private keys; the probability to create a zero field-element seems to be even many orders of magnitudes lower with 1/p)

  3. real-or-random commented at 8:43 AM on June 26, 2023: contributor

    Concept ACK

    Additionally, the first commit removes the function random_fe_test as it is logically a duplicate of random_field_element_test.

    I think it's better to use the name random_fe_test because we have other random_fe_* functions in the tests.

  4. theStack force-pushed on Jun 26, 2023
  5. theStack renamed this:
    tests: introduce helper for non-zero `random_field_element_test()` results
    tests: introduce helper for non-zero `random_fe_test()` results
    on Jun 26, 2023
  6. theStack commented at 1:56 PM on June 26, 2023: contributor

    Concept ACK

    Additionally, the first commit removes the function random_fe_test as it is logically a duplicate of random_field_element_test.

    I think it's better to use the name random_fe_test because we have other random_fe_* functions in the tests.

    Thanks, done. I had to move up the random_fe_test function in the code a little (before random_group_element_test) to avoid a forward declaration, let me know if using that instead would be preferred.

  7. real-or-random approved
  8. real-or-random commented at 1:58 PM on June 26, 2023: contributor

    utACK 5f9b7596cd31fac5f93ad3c5f0ba3dd1f613120f

  9. real-or-random added the label assurance on Jun 26, 2023
  10. real-or-random added the label refactor on Jun 26, 2023
  11. sipa commented at 6:12 PM on June 26, 2023: contributor

    utACK 5f9b7596cd31fac5f93ad3c5f0ba3dd1f613120f

  12. real-or-random commented at 7:38 AM on June 27, 2023: contributor

    needs rebase :/

  13. tests: refactor: remove duplicate function `random_field_element_test`
    There is a function `random_fe_test` which does exactly the
same, so use that instead. Note that it's also moved up before the
`random_group_element_test` function, in order to avoid needing a forward
declaration.
    304421d57b
  14. tests: introduce helper for non-zero `random_fe_test` results
    There are several instances in the tests where random non-zero field
elements are generated by calling `random_fe_test` in a do/while-loop.
This commit deduplicates all these by introducing a
`random_fe_non_zero_test` helper. Note that some instances checked the
is-zero condition via `secp256k1_fe_normalizes_to_zero_var`, which is
unnecessary, as the result of `random_fe_test` is already normalized (so
strictly speaking, this is not a pure refactor).
    5a95a268b9
  15. theStack force-pushed on Jun 27, 2023
  16. theStack commented at 8:42 AM on June 27, 2023: contributor

    needs rebase :/

    Done.

  17. real-or-random approved
  18. real-or-random commented at 10:16 AM on June 27, 2023: contributor

    ACK 5a95a268b944ffe64b7857e58f5b3b44aba514da

  19. real-or-random merged this on Jun 27, 2023
  20. real-or-random closed this on Jun 27, 2023
  21. theStack deleted the branch on Jun 27, 2023
  22. vmta referenced this in commit 8f03457eed on Jul 1, 2023
  23. fanquake referenced this in commit 56c05c5ec4 on Jul 17, 2023
  24. fanquake referenced this in commit ff061fde18 on Jul 18, 2023
  25. hebasto referenced this in commit 270d2b37b8 on Jul 21, 2023
  26. delta1 referenced this in commit 3f32c20932 on Aug 8, 2023
  27. delta1 referenced this in commit 31ac0c1081 on Aug 31, 2023
  28. janus referenced this in commit 476a2176e7 on Sep 11, 2023
  29. div72 referenced this in commit af627d47c3 on Apr 12, 2025
  30. str4d referenced this in commit 3b49801869 on Jun 4, 2025
Contributors
theStackreal-or-randomsipa
Labels
assurancetweak/refactor
github-metadata-mirror

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