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

pull theStack wants to merge 2 commits into bitcoin-core:master from theStack:add_missing_inverse_field_equal_checks changing 2 files +18 −18
  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

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-12-09 06:15 UTC

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