call fe_verfiy for not normalized field element in fe_set_b32 #1061

1. 
   siv2r commented at 10:27 pm on January 3, 2022: contributor

   https://github.com/bitcoin-core/secp256k1/blob/a1102b12196ea27f44d6201de4d25926a2ae9640/src/field_5x52_impl.h#L331-L339

   Shouldn’t the call to secp256k1_fe_verify come after the else block?

2. 
   real-or-random commented at 10:05 am on January 4, 2022: contributor

   This was changed in https://github.com/bitcoin-core/secp256k1/commit/34a67c773b0871e5797c7ab506d004e80911f120. I believe this is this intentionally to optimize the tests. Can you check if the tests take noticeably longer if the fe_verify is moved after the else block?
3. 
   siv2r commented at 3:20 am on January 11, 2022: contributor

   build commands: ./autogen.sh && ./configure && make -j13 Execution time (in seconds) of tests.c for three iterations (on 64-bit, i7-8750H). I used gettime_i64 function to measure the execution time. You can find my complete code here.

   branch	min	avg	max
   master (fe_verify inside if block)	145.94	146.26	146.72
   bench-tests (fe_verify after else block)	145.74	146.04	146.27

   There isn’t much difference in the execution time.

4. 
   real-or-random commented at 10:57 am on January 12, 2022: contributor

   Oh I think the time command would have been fine-grained enough for the tests. :)

   Yeah, I mean if the difference is below a second, I’d say it’s reasonable to always do the verify.

5. 
   siv2r commented at 12:05 pm on January 12, 2022: contributor

   Makes sense. The change required here is relatively small. I will check other parts of the code for similar issues and bundle them up in a PR.
6. 
   siv2r commented at 12:20 pm on January 12, 2022: contributor

   Oh I think the time command would have been fine-grained enough for the tests. :)

   I didn’t know such a command existed. I initially tried to use clock_gettime() for nanosecond precision..xD. It didn’t work since c89 does not seem to support it.

7. 
   real-or-random commented at 6:03 pm on January 18, 2022: contributor

   Maybe this could be solved in #1062 ? I think it will be similar to some changes proposed there.
8. siv2r cross-referenced this on Jan 26, 2022 from issue Removes `_fe_equal_var`, and unwanted `_fe_normalize_weak` calls (in tests) by siv2r
9. real-or-random referenced this in commit de657c2044 on Aug 17, 2023
10. 
    real-or-random commented at 9:02 am on March 5, 2024: contributor

    This has been resolved by #1062.
11. real-or-random closed this on Mar 5, 2024

    ---

Contributors
siv2r real-or-random