tests: add missing fe comparison checks for inverse field test cases #1489

1. 
   theStack commented at 0:15 am on February 1, 2024: contributor

   In the course of looking into #1472, I discovered the function check_fe_equal and noticed that at two call-sites (from 18 in total) there is no CHECK macro around, i.e. the return value is just thrown away and a failed test would go unnoticed. This seems to be easy to miss, as the name of the function suggests that a check happens, but it’s merely a wrapper around secp256k1_fe_equal that takes care of normalization.

   (Maybe doing the actual CHECK in check_fe_equal would be an option? On the other hand, I guess this makes it harder to spot the cause of a failed assertion, as the line number wouldn’t tell anything about from where the function was called.)

2. tests: add missing fe comparison checks for inverse field test cases

   `check_fe_equal` is a wrapper around `secp256k1_fe_equal` that takes
   care of normalization. Since it doesn't check anything itself, the
   CHECK macro is needed at the call-sites to actually ensure equality.

   00111c9c56
3. real-or-random added the label assurance on Feb 1, 2024
4. real-or-random added the label refactor/smell on Feb 1, 2024
5. 
   real-or-random commented at 9:06 am on February 1, 2024: contributor

   but it’s merely a wrapper around secp256k1_fe_equal that takes care of normalization.

   Perhaps we should just get rid of it. I don’t think it’s worth having a test function if it’s only used in two call sites, and anyway, people will forget that it exists when writing new tests.

6. 
   theStack commented at 10:06 am on February 1, 2024: contributor

   but it’s merely a wrapper around secp256k1_fe_equal that takes care of normalization.

   Perhaps we should just get rid of it. I don’t think it’s worth having a test function if it’s only used in two call sites, and anyway, people will forget that it exists when writing new tests.

   It’s used 18 times in total, it’s just in two of those cases the return value isn’t CHECKed (the ones that are touched in this PR). // EDIT: added the total number of call-sites to the PR description

7. 
   real-or-random commented at 1:27 pm on February 1, 2024: contributor

   Then I suggest renaming it just to fe_equal.

   (Maybe doing the actual CHECK in check_fe_equal would be an option? On the other hand, I guess this makes it harder to spot the cause of a failed assertion, as the line number wouldn’t tell anything about from where the function was called.)

   Yeah, I agree. Our idiom is to use CHECK and not have special check functions.

8. real-or-random cross-referenced this on Feb 1, 2024 from issue tests: Tidy the util functions by real-or-random
9. refactor: rename `check_fe_equal` -> `fe_equal`

   As this function doesn't do any checking, it's better to rename it,
   so that it's less likely to miss the needed `CHECK`.

   e7bdddd9c9
10. 
    theStack commented at 2:40 pm on February 1, 2024: contributor

    Then I suggest renaming it just to fe_equal.

    That seems a good idea. Done in another commit (can squash into one if desired).

11. real-or-random approved
12. 
    real-or-random commented at 2:51 pm on February 1, 2024: contributor

    utACK e7bdddd9c9c4b9f4f44420f3c7eeaef44f0c82ff
13. jonasnick approved
14. 
    jonasnick commented at 3:14 pm on February 27, 2024: contributor

    ACK e7bdddd9c9c4b9f4f44420f3c7eeaef44f0c82ff
15. jonasnick merged this on Feb 27, 2024
16. jonasnick closed this on Feb 27, 2024

    ---

17. theStack deleted the branch on Feb 27, 2024

Contributors
theStack real-or-random jonasnick

Labels
assurance refactor/smell