f can never equal -m #1603

pull roconnor-blockstream wants to merge 1 commits into bitcoin-core:master from roconnor-blockstream:f-is-not-neg-modulus_2024-09 changing 2 files +8 −12

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.
roconnor-blockstream force-pushed on Sep 6, 2024

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:

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

real-or-random approved
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
real-or-random added the label refactor/smell on Sep 25, 2024
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.
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.

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

roconnor-blockstream force-pushed on Sep 25, 2024
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.
real-or-random approved
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.)
real-or-random added the label assurance on Sep 26, 2024
sipa commented at 3:18 PM on October 7, 2024: contributor

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

Contributors

roconnor-blockstream

real-or-random

sipa

Labels

assurance refactor/smell