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. theStack cross-referenced this on Jun 27, 2023 from issue tighten group magnitude limits, save normalize_weak calls in group add methods (revival of #1032) by theStack
  23. vmta referenced this in commit 8f03457eed on Jul 1, 2023
  24. fanquake referenced this in commit 56c05c5ec4 on Jul 17, 2023
  25. fanquake referenced this in commit ff061fde18 on Jul 18, 2023
  26. hebasto referenced this in commit 270d2b37b8 on Jul 21, 2023

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: 2024-10-30 03:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me