Abstract out and merge all the magnitude/normalized logic

[sipa]

Right now, all the logic for propagating/computing the magnitude/normalized fields in secp256k1_fe (when VERIFY is defined) and the code for checking it, is duplicated across the two field implementations. I believe that is undesirable, as these properties should purely be a function of the performed fe_ functions, and not of the choice of field implementation. This becomes even uglier with #967, which would copy all that, and even needs an additional dimension that would then need to be added to the two other fields. It's also related to #1001, which I think will become easier if it doesn't need to be done/reasoned about separately for every field.

This PR moves all logic around these fields (collectively called field verification) to implementations in field_impl.h, which dispatch to renamed functions in field_*_impl.h for the actual implementation.

Fixes #1060.

[jonasnick]

PR looks good to me otherwise. Net negative diff 🎉

[robot-dreams]

Concept ACK

The process you used (rename fe_ to fe_impl, move verification into new fe_ functions in field_impl.h) makes sense.

I don't see any other relevant #ifdef VERIFY in field_10x26_impl.h or field_5x52_impl.h so it seems like you covered everything.

I just have a few clarifying questions, and then I want to do one final pass (maybe there's (1) some easy semi-automated way to check/reproduce the changes or (2) some more macros that can make field_impl.h shorter).

[sipa]

Made some significant changes here, and addressed some of the comments.

[robot-dreams]

New approach makes a lot of sense and commits are very well structured. Looks good overall, I just have a few cosmetic comments (feel free not to apply all of them).

[robot-dreams]

(Not this PR) Should normalized only be cleared if a > 1?

[sipa]

I'd instead rather add an VERIFY_CHECK(a > 1), because using this function with a=0 or a=1 is dumb.

[real-or-random]

I think it's fine. Making normalization depend on the value may be a step into the wrong direction (it makes magnitude less static, though the int argument probably doesn't matter.)

And I don't see a good reason to VERIFY_CHECK the input. a may be determined at run time. For example, we currently call this with 0 (determined at run time) in the tests. If you want to add the VERIFY_CHECK, you'd need to adjust random_field_element_magnitude.

[real-or-random]

Concept ACK

But is there you reason why you prefer to do this before #1001? (@peterdettman and I argued to in #1060 that it may make sense to solve #1001.)

I haven't looked at the details here but I think this will non-trivially conflict with #1062. @sipa can you have a look and suggest how we should move forward?

[sipa]

@real-or-random

But is there you reason why you prefer to do this before #1001? (@peterdettman and I argued to in #1060 that it may make sense to solve #1001.)

The impetus for this is really that I want to bring #967 up-to-date and more production-ready, and doing it properly I think would be very unclear without a change like this (having details from the "old" fields leak into the verify checks of the new one etc).

I'll comment in #1060, but I think this PR makes it at least obvious in what places norm/mag are not compile-time constants.

I haven't looked at the details here but I think this will non-trivially conflict with #1062. @sipa can you have a look and suggest how we should move forward?

It will. That's one of the reasons why I kept the changes for each function in separate commits. So I'm happy to rebase this after #1062, or otherwise incorporate the commits here.

[sipa]

Rebased.

[sipa]

Rebased, adding an abstraction for secp256k1_fe_half.

[sipa]

Rebased after merge of #1178.

[sipa]

Rebased after #1299, #1217, #979.

[sipa]

Made these changes:

diff --git a/src/field.h b/src/field.h
index 72e562e1..2c8fbc28 100644
--- a/src/field.h
+++ b/src/field.h
@@ -56,7 +56,7 @@
 #define SECP256K1_FE_VERIFY_CONST(d7, d6, d5, d4, d3, d2, d1, d0)
 #endif
 
-/** This expands to an initialized for a secp256k1_fe valued sum((i*32) * d_i, i=0..7) mod p.
+/** This expands to an initializer for a secp256k1_fe valued sum((i*32) * d_i, i=0..7) mod p.
  *
  * It has magnitude 1, unless d_i are all 0, in which case the magnitude is 0.
  * It is normalized, unless sum(2^(i*32) * d_i, i=0..7) >= p.
@@ -227,14 +227,14 @@ static void secp256k1_fe_add_int(secp256k1_fe *r, int a);
  * On input, r must be a valid field element. a must be an integer in [0,32].
  * The magnitude of r times a must not exceed 32.
  * Performs {r *= a}.
- * On output, r's magnitude is multiplied by a, and normalized is cleared.
+ * On output, r's magnitude is multiplied by a, and r will not be normalized.
  */
 static void secp256k1_fe_mul_int(secp256k1_fe *r, int a);
 
 /** Increment a field element by another.
  *
  * On input, r and a must be valid field elements, not necessarily normalized.
- * The sum of their magnitudes may not exceed 32.
+ * The sum of their magnitudes must not exceed 32.
  * Performs {r += a}.
  * On output, r will not be normalized, and will have magnitude incremented by a's.
  */
@@ -309,9 +309,12 @@ static void secp256k1_fe_storage_cmov(secp256k1_fe_storage *r, const secp256k1_f
  */
 static void secp256k1_fe_cmov(secp256k1_fe *r, const secp256k1_fe *a, int flag);
 
-/** Halves the value of a field element modulo the field prime. Constant-time.
- *  For an input magnitude 'm', the output magnitude is set to 'floor(m/2) + 1'.
- *  The output is not guaranteed to be normalized, regardless of the input. */
+/** Halve the value of a field element modulo the field prime in constant-time.
+ *
+ * On input, r must be a valid field element.
+ * On output, r will be normalized and have magnitude floor(m/2) + 1 where m is
+ * the magnitude of r on input.
+ */
 static void secp256k1_fe_half(secp256k1_fe *r);
 
 /** Sets r to a field element with magnitude m, normalized if (and only if) m==0.
@@ -319,7 +322,10 @@ static void secp256k1_fe_half(secp256k1_fe *r);
  *  internal overflows. */
 static void secp256k1_fe_get_bounds(secp256k1_fe *r, int m);
 
-/** Determine whether a is a square (modulo p). */
+/** Determine whether a is a square (modulo p).
+ *
+ * On input, a must be a valid field element.
+ */
 static int secp256k1_fe_is_square_var(const secp256k1_fe *a);
 
 /** Check invariants on a field element (no-op unless VERIFY is enabled). */
diff --git a/src/field_impl.h b/src/field_impl.h
index 630c9dca..c082d8f5 100644
--- a/src/field_impl.h
+++ b/src/field_impl.h
@@ -55,7 +55,7 @@ static int secp256k1_fe_sqrt(secp256k1_fe *r, const secp256k1_fe *a) {
      *  itself always a square (a ** ((p+1)/4) is the square of a ** ((p+1)/8)).
      */
     secp256k1_fe x2, x3, x6, x9, x11, x22, x44, x88, x176, x220, x223, t1;
-    int j;
+    int j, ret;
 
 #ifdef VERIFY
     VERIFY_CHECK(r != a);
@@ -145,7 +145,16 @@ static int secp256k1_fe_sqrt(secp256k1_fe *r, const secp256k1_fe *a) {
     /* Check that a square root was actually calculated */
 
     secp256k1_fe_sqr(&t1, r);
-    return secp256k1_fe_equal(&t1, a);
+    ret = secp256k1_fe_equal(&t1, a);
+
+#ifdef VERIFY
+    if (!ret) {
+        secp256k1_fe_negate(&t1, &t1, 1);
+        secp256k1_fe_normalize_var(&t1);
+        VERIFY_CHECK(secp256k1_fe_equal_var(&t1, a));
+    }
+#endif
+    return ret;
 }
 
 #ifndef VERIFY

[jonasnick]

ACK 7fc642f

[real-or-random]

ACK 7fc642f