Merge bitcoin-core/secp256k1#1317: Make fe_cmov take max of magnitudes

31b4bbe Make fe_cmov take max of magnitudes (Pieter Wuille)

Pull request description:

  This addresses part of bitcoin#1001.

  The magnitude and normalization of the output of `secp256k1_fe_cmov` should not depend on the runtime value of `flag`.

ACKs for top commit:
  real-or-random:
    utACK 31b4bbe
  stratospher:
    ACK 31b4bbe.

Tree-SHA512: 08bef9f63797cb8a1f3ea63c716c09aaa267dfee285b74ef5fbb47d614569d2787ec73d21bce080214872dfe70246f73cea42ad3c24e6baccecabe3312f71433

src/field.h

@@ -310,7 +310,9 @@ static void secp256k1_fe_storage_cmov(secp256k1_fe_storage *r, const secp256k1_f
  *
  * On input, both r and a must be valid field elements. Flag must be 0 or 1.
  * Performs {r = flag ? a : r}.
- * On output, r's magnitude and normalized will equal a's in case of flag=1, unchanged otherwise.
+ *
+ * On output, r's magnitude will be the maximum of both input magnitudes.
+ * It will be normalized if and only if both inputs were normalized.
  */
 static void secp256k1_fe_cmov(secp256k1_fe *r, const secp256k1_fe *a, int flag);
 

src/field_impl.h

@@ -352,10 +352,8 @@ SECP256K1_INLINE static void secp256k1_fe_cmov(secp256k1_fe *r, const secp256k1_
     secp256k1_fe_verify(a);
     secp256k1_fe_verify(r);
     secp256k1_fe_impl_cmov(r, a, flag);
-    if (flag) {
-        r->magnitude = a->magnitude;
-        r->normalized = a->normalized;
-    }
+    if (a->magnitude > r->magnitude) r->magnitude = a->magnitude;
+    if (!a->normalized) r->normalized = 0;
     secp256k1_fe_verify(r);
 }
 

src/tests.c

@@ -3101,10 +3101,6 @@ static void run_field_be32_overflow(void) {
 /* Returns true if two field elements have the same representation. */
 static int fe_identical(const secp256k1_fe *a, const secp256k1_fe *b) {
     int ret = 1;
-#ifdef VERIFY
-    ret &= (a->magnitude == b->magnitude);
-    ret &= (a->normalized == b->normalized);
-#endif
     /* Compare the struct member that holds the limbs. */
     ret &= (secp256k1_memcmp_var(a->n, b->n, sizeof(a->n)) == 0);
     return ret;
@@ -3192,16 +3188,22 @@ static void run_field_misc(void) {
         q = x;
         secp256k1_fe_cmov(&x, &z, 0);
 #ifdef VERIFY
-        CHECK(x.normalized && x.magnitude == 1);
+        CHECK(!x.normalized);
+        CHECK((x.magnitude == q.magnitude) || (x.magnitude == z.magnitude));
+        CHECK((x.magnitude >= q.magnitude) && (x.magnitude >= z.magnitude));
 #endif
+        x = q;
         secp256k1_fe_cmov(&x, &x, 1);
         CHECK(!fe_identical(&x, &z));
         CHECK(fe_identical(&x, &q));
         secp256k1_fe_cmov(&q, &z, 1);
 #ifdef VERIFY
-        CHECK(!q.normalized && q.magnitude == z.magnitude);
+        CHECK(!q.normalized);
+        CHECK((q.magnitude == x.magnitude) || (q.magnitude == z.magnitude));
+        CHECK((q.magnitude >= x.magnitude) && (q.magnitude >= z.magnitude));
 #endif
         CHECK(fe_identical(&q, &z));
+        q = z;
         secp256k1_fe_normalize_var(&x);
         secp256k1_fe_normalize_var(&z);
         CHECK(!secp256k1_fe_equal_var(&x, &z));
@@ -3215,7 +3217,7 @@ static void run_field_misc(void) {
             secp256k1_fe_normalize_var(&q);
             secp256k1_fe_cmov(&q, &z, (j&1));
 #ifdef VERIFY
-            CHECK((q.normalized != (j&1)) && q.magnitude == ((j&1) ? z.magnitude : 1));
+            CHECK(!q.normalized && q.magnitude == z.magnitude);
 #endif
         }
         secp256k1_fe_normalize_var(&z);
@@ -7558,23 +7560,23 @@ static void fe_cmov_test(void) {
     secp256k1_fe a = zero;
 
     secp256k1_fe_cmov(&r, &a, 0);
-    CHECK(secp256k1_memcmp_var(&r, &max, sizeof(r)) == 0);
+    CHECK(fe_identical(&r, &max));
 
     r = zero; a = max;
     secp256k1_fe_cmov(&r, &a, 1);
-    CHECK(secp256k1_memcmp_var(&r, &max, sizeof(r)) == 0);
+    CHECK(fe_identical(&r, &max));
 
     a = zero;
     secp256k1_fe_cmov(&r, &a, 1);
-    CHECK(secp256k1_memcmp_var(&r, &zero, sizeof(r)) == 0);
+    CHECK(fe_identical(&r, &zero));
 
     a = one;
     secp256k1_fe_cmov(&r, &a, 1);
-    CHECK(secp256k1_memcmp_var(&r, &one, sizeof(r)) == 0);
+    CHECK(fe_identical(&r, &one));
 
     r = one; a = zero;
     secp256k1_fe_cmov(&r, &a, 0);
-    CHECK(secp256k1_memcmp_var(&r, &one, sizeof(r)) == 0);
+    CHECK(fe_identical(&r, &one));
 }
 
 static void fe_storage_cmov_test(void) {