Have ge_set_gej_var, gej_double_var and ge_set_all_gej_var initialize all fields of their outputs. #937

1. 
   roconnor-blockstream commented at 7:03 pm on May 4, 2021: contributor

   Previous behaviour would not initialize r->x and r->y values in the case where infinity is passed in.
2. Have secp256k1_ge_set_gej_var initialize all fields.

   Previous behaviour would not initialize r->x and r->y values in the case where infinity is passed in.

   dd6c3de322
3. 
   gmaxwell commented at 7:07 pm on May 4, 2021: contributor

   utACK dd6c3de322740a3054cf6a1994a38dc8f201b473
4. 
   real-or-random commented at 7:28 pm on May 4, 2021: contributor

   Concept ACK

   Unless I’m mistaken, I think VG_UNDEFs here would be helpful. See #889 (comment).

5. 
   gmaxwell commented at 7:31 pm on May 4, 2021: contributor

   This function exits without leaving anything uninitialized, so nothing to vg_undef here. Aside, in the past I hadn’t UNDEFed anywhere in the codebase itself, only in the tests to avoid injecting vg instrumentation in the production binary.. but we do that now for the CT tests and I think were were satisfied it was benign enough.

   edit: oh I see, the set infinity could be undefing x/y. I suppose it could be. But that should be a separate PR if it gets done.

6. 
   real-or-random commented at 7:34 pm on May 4, 2021: contributor

   edit: oh I see, the set infinity could be undefing x/y. I suppose it could be.

   Right, that’s what I had in mind.

   But that should be a separate PR if it gets done.

   Fair point.

7. real-or-random approved
8. 
   real-or-random commented at 7:36 pm on May 4, 2021: contributor

   utACK dd6c3de322740a3054cf6a1994a38dc8f201b473
9. Have secp256k1_gej_double_var initialize all fields.

   Previous behaviour would not initialize r->x and r->y values in the case where infinity is passed in.

   31c0f6de41
10. roconnor-blockstream renamed this:
    Have secp256k1_ge_set_gej_var initialize all fields.
    Have secp256k1_ge_set_gej_var and secp256k1_gej_double_var initialize all fields.
    on May 4, 2021
11. Have secp256k1_ge_set_all_gej_var initialize all fields.

    Previous behaviour would not initialize r->y values in the case where infinity is passed in.
    Furthermore, the previous behaviour wouldn't initialize anything in the case where all inputs were infinity.

    45b6468d7e
12. roconnor-blockstream renamed this:
    Have secp256k1_ge_set_gej_var and secp256k1_gej_double_var initialize all fields.
    Have ge_set_gej_var, gej_double_var and ge_set_all_gej_var initialize all fields of their outputs.
    on May 4, 2021
13. 
    gmaxwell commented at 11:50 pm on May 4, 2021: contributor

    utACK 45b6468d7e3ed9849ed474c71e9a9479de1a77db
14. 
    real-or-random commented at 10:33 am on May 5, 2021: contributor

    @roconnor-blockstream You could cherry pick the top two commits from https://github.com/real-or-random/secp256k1/commits/20210504_ge_set_gej_var

    edit: I have verified that the new test fails on master.

15. roconnor-blockstream force-pushed on May 5, 2021
16. 
    real-or-random commented at 2:43 pm on May 5, 2021: contributor

    Ah damn, this was not the latest version on my branch -.- Let me push the right one. Sorry.
17. 
    real-or-random commented at 3:19 pm on May 5, 2021: contributor

    Now my branch should have the right commits… I somehow had lost them. (The reflog is nice sometimes. :))
18. roconnor-blockstream force-pushed on May 5, 2021
19. roconnor-blockstream force-pushed on May 5, 2021
20. tests: Test secp256k1_ge_set_all_gej_var for all infinity inputs 4a19668c37
21. tests: Improve secp256k1_ge_set_all_gej_var for some infinity inputs 14c9739a1f
22. 
    gmaxwell commented at 6:14 pm on May 5, 2021: contributor

    ACK 14c9739a1fb485bb56dbe3447132a37bcbef4e22
23. in src/tests.c:3109 in 14c9739a1f

    3105         random_group_element_test(&ge[i]);
    3106+        odd = secp256k1_fe_is_odd(&ge[i].x);
    3107+        CHECK(odd == 0 || odd == 1);
    3108         /* randomly set half the points to infinity */
    3109-        if(secp256k1_fe_is_odd(&ge[i].x)) {
    3110+        if (odd == i % 2) {
    

    ---

    ---

    

    sipa commented at 6:35 pm on May 5, 2021:

    Nit: % 2 seems superfluous here.

    ---

    

    real-or-random commented at 7:00 pm on May 5, 2021:

    Ah I guess it depends on the thing you want to test.

    Before the change, the input consisted of some infinity points and some affine points with even x coordinate. After the change, the input consists of some infinity points and some (arbitrary) affine points.

    ---

    

    sipa commented at 0:33 am on May 6, 2021:

    Well the line before is CHECK(odd == 0 || odd == 1);. The % 2 is guaranteed to not have any effect.

    ---

    

    sipa commented at 2:18 am on May 6, 2021:

    Nevermind, I was just misreading this line as if (odd % 2) {.
24. 
    sipa commented at 6:36 pm on May 5, 2021: contributor

    utACK 14c9739a1fb485bb56dbe3447132a37bcbef4e22
25. real-or-random approved
26. 
    real-or-random commented at 7:37 am on May 6, 2021: contributor

    ACK 14c9739a1fb485bb56dbe3447132a37bcbef4e22
27. real-or-random merged this on May 6, 2021
28. real-or-random closed this on May 6, 2021

    ---

29. jonasnick cross-referenced this on Jun 14, 2021 from issue Upstream PRs 831, 907, 903, 889, 918, 906, 928, 922, 933, Merge bitcoin-core/secp256k1#936: Fix gen_context/ASM build on ARM, 925, 937, 926, Merge bitcoin-core/secp256k1#940: contrib: Explain explicit header guards, 850, 930, 941, 846, 947, 662, 950 by jonasnick

Contributors
roconnor-blockstream gmaxwell real-or-random sipa