fe_sqr tests could be improved #1472

issue Coding-Enthusiast openend this issue on December 24, 2023
  1. Coding-Enthusiast commented at 5:41 am on December 24, 2023: contributor

    https://github.com/bitcoin-core/secp256k1/blob/efe85c70a2e357e3605a8901a9662295bae1001f/src/tests.c#L3289-L3303

    This method doesn’t test anything and after looking through commits it was added in https://github.com/bitcoin-core/secp256k1/commit/59447da38d6fa394ab7b10f32aea55930da732be and got removed in the next commit https://github.com/bitcoin-core/secp256k1/commit/21f81a846957408d4a2fe30cff32370567887a93. I’m not sure what the “problem” mentioned in the first commit is but if it is checking it using the SECP256K1_FE_VERIFY method, it is pointless to call run_sqr without this build configuration.

  2. real-or-random commented at 5:23 pm on December 24, 2023: contributor

    it is pointless to call run_sqr without this build configuration.

    Well, there’s nothing wrong with it, except that it adds a few milliseconds to the noverify_tests. I claim that adding #ifdefs isn’t worth the complexity and the development time. So this is not an issue.

    The test itself could perhaps be improved. We could compare with the result of another method, e.g., fe_mul. We can’t use fe_mul to compute x*x directly (see docs) but we could, e.g., check that (x+y)*(x-y) = x^2 - y^2 for some random values y != 0.

  3. real-or-random added the label assurance on Dec 24, 2023
  4. real-or-random added the label refactor/smell on Dec 24, 2023
  5. real-or-random commented at 0:08 am on January 9, 2024: contributor

    @Coding-Enthusiast

    we could, e.g., check that (x+y)*(x-y) = x^2 - y^2 for some random values y != 0.

    Do you want to give it a try and work on a PR?

  6. real-or-random renamed this:
    run_sqr(void) in tests needs #ifdef VERIFY
    fe_sqr tests could be improved
    on Jan 9, 2024
  7. Coding-Enthusiast commented at 2:18 pm on January 9, 2024: contributor
    Sadly my C skills are readonly 😊
  8. real-or-random commented at 5:58 pm on January 9, 2024: contributor
    We all started small… ;)
  9. theStack cross-referenced this on Feb 1, 2024 from issue tests: add missing fe comparison checks for inverse field test cases by theStack
  10. theStack cross-referenced this on Feb 1, 2024 from issue tests: improve fe_sqr test (issue #1472) by theStack
  11. jonasnick referenced this in commit 427e86b9ed on Feb 27, 2024
  12. real-or-random commented at 8:19 am on March 5, 2024: contributor
    This has been resolved by #1490.
  13. real-or-random closed this on Mar 5, 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-10-31 23:15 UTC

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