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 12: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. 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
  9. 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).

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

    utACK e7bdddd9c9c4b9f4f44420f3c7eeaef44f0c82ff

  12. jonasnick approved
  13. jonasnick commented at 3:14 PM on February 27, 2024: contributor

    ACK e7bdddd9c9c4b9f4f44420f3c7eeaef44f0c82ff

  14. jonasnick merged this on Feb 27, 2024
  15. jonasnick closed this on Feb 27, 2024
  16. theStack deleted the branch on Feb 27, 2024
  17. achow101 referenced this in commit b525369f8e on Mar 25, 2024
  18. achow101 referenced this in commit 2189d2f841 on Apr 1, 2024
  19. fanquake referenced this in commit 5354807b00 on Apr 4, 2024
  20. fanquake referenced this in commit 53eec53dca on Apr 4, 2024
  21. hebasto referenced this in commit b6de625950 on May 11, 2024
  22. vmta referenced this in commit 2ef958bb88 on May 20, 2024
  23. janus referenced this in commit 939262869a on Jun 6, 2024
  24. div72 referenced this in commit af627d47c3 on Apr 12, 2025
  25. oskarszoon referenced this in commit 0d5c9260f4 on Jul 1, 2025
  26. real-or-random referenced this in commit 7e460db4ca on Jan 29, 2026
  27. github-actions[bot] referenced this in commit 758d4e90b4 on Mar 1, 2026
  28. github-actions[bot] referenced this in commit 68a2178f22 on Mar 1, 2026
  29. github-actions[bot] referenced this in commit a8bc1a0b2b on Mar 1, 2026
  30. 0x000000000019d6689c085ae165831e934ff76 referenced this in commit 3b9450150d on Mar 2, 2026
  31. csjones referenced this in commit a4d92824ae on Mar 2, 2026
Contributors
theStackreal-or-randomjonasnick
Labels
assurancetweak/refactor
Linked (view graph)
#1472 fe_sqr tests could be improved#1491 tests: Tidy the util functions
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