field: Improve docs +tests of secp256k1_fe_set_b32 #1205

1. 
   real-or-random commented at 11:31 am on February 1, 2023: contributor

   We could also change the interface to accept an explicit overflow output variable, like for the scalar function. I think this makes handling (and ignoring!) overflow much more explicit in the code. What do you think?
2. field: Verify field element even after secp256k1_fe_set_b32 fails d93f62e369
3. field: Simplify code in secp256k1_fe_set_b32 ca92a35d01
4. real-or-random force-pushed on Feb 1, 2023
5. real-or-random force-pushed on Feb 1, 2023
6. 
   sipa commented at 8:34 pm on February 1, 2023: contributor

   Concept ACK, though is the inclusion of the last commit intentional?

   An alternative follow-up could be to split the function in two:

   • One that doesn’t return whether overflow occurred, and only promises weak normalization.
   • One that fails in case overflow occurred (and returns it), and promises strong normalization.

   This would get rid of the fact that this is one of the few places in the codebase where field magnitude/normalization properties depend on runtime values (they’re almost always compile-time known otherwise).

7. 
   real-or-random commented at 8:57 am on February 2, 2023: contributor

   Concept ACK, though is the inclusion of the last commit intentional?

   Yes, but I’m happy to remove it, of course. The story behind this is that I had forgotten to remove a debug printer first when I opened this PR. I mentioned this to @jonasnick who told me that he also had to write these functions multiple times in the past, so we thought it’s a good idea to have one in the code base. Then I made it a bit nicer and added a commit. And I felt it’s not worth a separate PR (but maybe I was just being lazy there).

8. 
   real-or-random commented at 8:59 am on February 2, 2023: contributor

   An alternative follow-up could be to split the function in two: […]

   That’s a great idea.

   We could still have overflow as an explicit argument of the (second) function. What do you think?

9. 
   sipa commented at 3:20 pm on February 2, 2023: contributor

   We could still have overflow as an explicit argument of the (second) function. What do you think?

   It could, but I think that’d be confusing. The second function cannot handle overflow; if it was provided input that’s out of range, it should return failure, and not make any promises about the parsed secp256k1_fe. The caller is supposed to return or handle failure in this case.

10. 
    real-or-random commented at 4:09 pm on February 2, 2023: contributor

    Ok yes, understood now.
11. 
    sipa commented at 9:16 pm on February 2, 2023: contributor

    utACK 727d50aaaee6dbadee3b9395b9141783607e5f69
12. sipa cross-referenced this on Feb 5, 2023 from issue Split fe_set_b32 into reducing and normalizing variants by sipa
13. in src/tests.c:2992 in 727d50aaae outdated

    2987+        static const unsigned char zero[32] = {
    2988+            0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
    2989+            0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
    2990+            0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
    2991+            0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
    2992+        };
    

    ---

    ---

    

    jonasnick commented at 3:33 pm on February 6, 2023:

    nit: fewer loc, easier to check

    0        static const unsigned char zero[32] = { 0x00 };
    

14. in src/tests.c:54 in 727d50aaae outdated

    43@@ -44,6 +44,21 @@ static int all_bytes_equal(const void* s, unsigned char value, size_t n) {
    44     return 1;
    45 }
    46 
    47+/* Debug helper for printing arrays of unsigned char. */
    48+static void print_buf(const unsigned char *buf, size_t len) {
    49+    size_t i;
    50+    printf("{");
    

    ---

    ---

    

    jonasnick commented at 3:35 pm on February 6, 2023:

    This would be extra useful if this helper would also output the name of the printed variable. If print_buf was a macro we could do it like this:

    0    printf("%s = {", #buf);
    

15. 
    jonasnick commented at 3:37 pm on February 6, 2023: contributor

    ACK mod nits
16. field: Improve docs and tests of secp256k1_fe_set_b32 e9fd3dff76
17. tests: Add debug helper for printing buffers 162da73e9a
18. real-or-random force-pushed on Apr 21, 2023
19. 
    real-or-random commented at 3:21 pm on April 21, 2023: contributor

    force-pushed to address @jonasnick’s nits
20. 
    jonasnick commented at 4:07 pm on April 21, 2023: contributor

    ACK 162da73e9a48875aab1ee6ca1c14f86ca4646946
21. jonasnick merged this on Apr 21, 2023
22. jonasnick closed this on Apr 21, 2023

    ---

23. jonasnick cross-referenced this on Apr 23, 2023 from issue tests: remove extra semicolon in macro by jonasnick
24. sipa referenced this in commit 3353d3c753 on May 11, 2023
25. sipa referenced this in commit b4eb644b6c on May 12, 2023
26. hebasto referenced this in commit 49c52ea2b1 on May 13, 2023
27. vmta referenced this in commit e1120c94a1 on Jun 4, 2023
28. sipa referenced this in commit 901336eee7 on Jun 21, 2023
29. vmta referenced this in commit 8f03457eed on Jul 1, 2023
30. jonasnick cross-referenced this on Jul 24, 2023 from issue Upstream PRs 1268, 1276, 1267, 1265, 1230, 1279, 1273, 1263, 1231, 1285, 1283, 1205, 1286, 1275, 1234, 1239, 1240, 1284, 1277, 1289, 1270, 1296, 1301, 1299, 1066, 1300, 1292, 1305, 1303, 1133, 1306, 1207, 1304, 1307, 1311, 1309, 1312 by jonasnick

Contributors
real-or-random sipa jonasnick