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

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
27. jonasnick cross-referenced this on Jul 27, 2023 from issue Upstream PRs 1357, 1345, 1358, 1368, 1369, 1323, 1364, 1370, 1367, 1344, 1371, 1363, 1340, 1366, 1313, 1378, 1298, 1382, 1383, 1384, 1386 by jonasnick

Contributors
theStack real-or-random sipa

Labels
assurance refactor/smell