f can never equal -m #1603

1. 
   roconnor-blockstream commented at 3:25 pm on September 6, 2024: contributor

   In fact, before reaching this particular VERIFY_CHECK, we had already successfully passed through

   VERIFY_CHECK(secp256k1_modinv64_mul_cmp_62(&f, len, &modinfo->modulus, -1) > 0); /* f > -modulus */
   

   ensuring that f is not -m.

2. roconnor-blockstream force-pushed on Sep 6, 2024
3. in src/modinv32_impl.h:646 in e53ed2efeb outdated

   642@@ -643,13 +643,12 @@ static void secp256k1_modinv32_var(secp256k1_modinv32_signed30 *x, const secp256
   643 
   644     /* g == 0 */
   645     VERIFY_CHECK(secp256k1_modinv32_mul_cmp_30(&g, len, &SECP256K1_SIGNED30_ONE, 0) == 0);
   646-    /* |f| == 1, or (x == 0 and d == 0 and |f|=modulus) */
   647+    /* |f| == 1, or (x == 0 and d == 0 and f=modulus) */
   

   ---

   ---

   

   real-or-random commented at 1:33 pm on September 9, 2024:

   micro-nit:

   0    /* |f| == 1, or (x == 0 and d == 0 and f == modulus) */
   

4. real-or-random approved
5. 
   real-or-random commented at 1:32 pm on September 25, 2024: contributor

   Okay yes. All VERIFY_CHECKs are redundant in a sense, but this one is really covered by another VERIFY_CHECK already.

   utACK e53ed2efebcca6483a511ac0d9f3162973d66dda

6. real-or-random added the label refactor/smell on Sep 25, 2024
7. 
   roconnor-blockstream commented at 2:00 pm on September 25, 2024: contributor

   Just to be clear, this PR isn’t removing a redundant check, it is strengthening the existing check by removing a disjunctive clause.
8. 
   real-or-random commented at 2:46 pm on September 25, 2024: contributor

   Just to be clear, this PR isn’t removing a redundant check, it is strengthening the existing check by removing a disjunctive clause.

   Oh, right, I got this wrong. But I still think the change makes sense.

9. f can never equal -m

   In fact, before reaching this particular VERIFY_CHECK, we had already successfully passed through
   
       VERIFY_CHECK(secp256k1_modinv64_mul_cmp_62(&f, len, &modinfo->modulus, -1) > 0); /* f > -modulus */
   
   ensuring that f is not -m.

   ef7ff03407
10. roconnor-blockstream force-pushed on Sep 25, 2024
11. 
    roconnor-blockstream commented at 3:06 pm on September 25, 2024: contributor

    Done. I’ve also added the same change to the constant-time versions of these functions.
12. real-or-random approved
13. 
    real-or-random commented at 3:16 pm on September 25, 2024: contributor

    utACK ef7ff03407fa79e7eab1b6771b77f72828e63636

    (There seems to be some CI hiccup unrelated to this PR.)

14. real-or-random added the label assurance on Sep 26, 2024
15. 
    sipa commented at 3:18 pm on October 7, 2024: contributor

    ACK ef7ff03407fa79e7eab1b6771b77f72828e63636
16. real-or-random merged this on Oct 8, 2024
17. real-or-random closed this on Oct 8, 2024

    ---

Contributors
roconnor-blockstream real-or-random sipa

Labels
assurance refactor/smell