tighten group magnitude limits, save normalize_weak calls in group add methods (revival of #1032)

[theStack]

This PR picks up #1032 by peterdettman. It's essentially a rebase on master; the original first commit (09dbba5) which introduced group verification methods has mostly been replaced by PR #1299 (commit f202667) and what remains now is only adding a few missing checks at some places. The remaining commits are unchanged, though some (easy-to-solve) conflicts appeared through cherry-picking. The last commit which actually removes the normalize_weak calls is obviously the critical one and needs the most attention for review.

[theStack]

Rebased on master (there was a conflict in tests.c due to #1358) and also updated the operations count comments on each of the optimized functions (i.e. secp256k1_gej_add_ge_var, secp256k1_gej_add_zinv_var and secp256k1_gej_add_ge) in the last commit.

[real-or-random]

ACK mod nits

[real-or-random]

I think some of the existing VERIFY_CHECKs in field_impl.h can be replaced by calls to secp256k1_fe_verify_magnitude (If that's desirable but I guess so?)

[theStack]

Force-pushed with most suggestions tackled.

I think some of the existing VERIFY_CHECKs in field_impl.h can be replaced by calls to secp256k1_fe_verify_magnitude (If that's desirable but I guess so?)

Added another commit for doing that after the "secp256k1_ge_table_set_globalz" one (203e41b86280d5ea5ed9ab599ddbc21970529601).

[real-or-random]

Although that could be done in a separate PR, I think this ideally should also remove the restriction to magnitude 31 introduced here: 07c0e8b#diff-28d9b92418cbddbe487fe84847993cc5da0b096363c83631a525cff25189752cR104 This is anyway now implied by the new limits.

Possible follow-up PRs/issues:

• Some (but not all) of the group functions verify their input fes. That doesn't hurt, but it's also not necessary because all field functions do that anyway. Anyway, we should either always do it or never.
• We should really think about #ifdef VERIFY now. It seems that the current convention is to put all new VERIFY_CHECKs in ifdefs. If we do this anyway, then we may as well redefine VERIFY_CHECK to be a noop (and give up on the idea that this macro is "side-effect safe". Another consideration is readability the uppercase VERIFY stands out. With more functions like ge_verify, it's a bit hard to spot them, so maybe we should actually keep the ifdefs. Or rename the functions to fe_VERIFY.

[theStack]

Thanks for the review. Force-pushed, removed the <= 31 magnitude check in secp256k1_gej_eq_x_var (included in the commit which introduces the tighter group magnitude limits) and also tackled the two other smaller review comments above.

Some comments to the follow-up ideas:

Some (but not all) of the group functions verify their input fes. That doesn't hurt, but it's also not necessary because all field functions do that anyway. Anyway, we should either always do it or never.

Any preference between "always" and "never"? Since the checks are not necessary, I would personally prefer "never", leading to less code, even though the diff would be a bit larger (there are more functions taking fe inputs in group_impl.h that already verify the fe than those that miss the check), but that shouldn't matter.

As for the #ifdef VERIFY subject, what exactly does "side-effect safe" mean in this context, is the point here to allow side-effects, and that's why the expression also needs to be executed in non-verify mode? I fail to see why having side-effects in checks would ever be useful or generally desirable (quickly glancing over git grep VERIFY_CHECK results also didn't show me any expressions with side-effects, but maybe I'm missing something), so your proposal of redefining VERIFY_CHECK to a no-op in non-verify mode sounds reasonable to me.

[real-or-random]

ACK 31bc9a4

[real-or-random]

Some (but not all) of the group functions verify their input fes. That doesn't hurt, but it's also not necessary because all field functions do that anyway. Anyway, we should either always do it or never.

Any preference between "always" and "never"? Since the checks are not necessary, I would personally prefer "never", leading to less code, even though the diff would be a bit larger (there are more functions taking fe inputs in group_impl.h that already verify the fe than those that miss the check), but that shouldn't matter.

I tend to agree. This will also separate concerns more clearly: The field code verifies the field element, the group code verifies the group elements. And if the group code accesses fe internals directly (e.g. magnitude), that should be considered a layer violation (and this PR here will fix this by adding secp256k1_fe_verify_magnitude).

As for the #ifdef VERIFY subject, [...]

See #1381.

[theStack]

@jonasnick: Thanks for the review! I've tried to tackle your suggestions in a separate branch for review of how the proposed changes would look like (https://github.com/theStack/secp256k1/tree/revival_of_1032_magnitude_comments, in particular the last commit theStack@b7c685e).
This is the range-diff from the current version (which already has one ACK) to the one with tighter limits and magnitude annotations expressed with limit variables (git range-diff 31bc9...b7c68):

Diff

1:  d4527d6 ! 1:  c83afa6 Tighten group magnitude limits
    @@ Commit message
         - adjust test methods that randomize magnitudes
     
         Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
    +    Co-authored-by: Jonas Nick <jonasd.nick@gmail.com>
     
      ## src/group.h ##
     @@ src/group.h: typedef struct {
    @@ src/group.h: typedef struct {
     -#define SECP256K1_GEJ_X_MAGNITUDE_MAX 8
     -#define SECP256K1_GEJ_Y_MAGNITUDE_MAX 8
     -#define SECP256K1_GEJ_Z_MAGNITUDE_MAX 8
    -+#define SECP256K1_GE_X_MAGNITUDE_MAX  6
    -+#define SECP256K1_GE_Y_MAGNITUDE_MAX  4
    -+#define SECP256K1_GEJ_X_MAGNITUDE_MAX 6
    ++#define SECP256K1_GE_X_MAGNITUDE_MAX  4
    ++#define SECP256K1_GE_Y_MAGNITUDE_MAX  3
    ++#define SECP256K1_GEJ_X_MAGNITUDE_MAX 4
     +#define SECP256K1_GEJ_Y_MAGNITUDE_MAX 4
    -+#define SECP256K1_GEJ_Z_MAGNITUDE_MAX 2
    ++#define SECP256K1_GEJ_Z_MAGNITUDE_MAX 1
      
      /** Set a group element equal to the point with given X and Y coordinates */
      static void secp256k1_ge_set_xy(secp256k1_ge *r, const secp256k1_fe *x, const secp256k1_fe *y);
2:  31bc9a4 ! 2:  b7c685e Save _normalize_weak calls in group add methods
    @@ Commit message
     
         Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
         Co-authored-by: Tim Ruffing <crypto@timruffing.de>
    +    Co-authored-by: Jonas Nick <jonasd.nick@gmail.com>
     
      ## src/group_impl.h ##
     @@ src/group_impl.h: static void secp256k1_gej_add_var(secp256k1_gej *r, const secp256k1_gej *a, cons
    @@ src/group_impl.h: static void secp256k1_gej_add_ge_var(secp256k1_gej *r, const s
     +    s1 = a->y;
          secp256k1_fe_mul(&s2, &b->y, &z12); secp256k1_fe_mul(&s2, &s2, &a->z);
     -    secp256k1_fe_negate(&h, &u1, 1); secp256k1_fe_add(&h, &u2);
    -+    secp256k1_fe_negate(&h, &u1, 6); secp256k1_fe_add(&h, &u2);
    ++    secp256k1_fe_negate(&h, &u1, SECP256K1_GEJ_X_MAGNITUDE_MAX); secp256k1_fe_add(&h, &u2);
          secp256k1_fe_negate(&i, &s2, 1); secp256k1_fe_add(&i, &s1);
          if (secp256k1_fe_normalizes_to_zero_var(&h)) {
              if (secp256k1_fe_normalizes_to_zero_var(&i)) {
    @@ src/group_impl.h: static void secp256k1_gej_add_zinv_var(secp256k1_gej *r, const
     +    s1 = a->y;
          secp256k1_fe_mul(&s2, &b->y, &z12); secp256k1_fe_mul(&s2, &s2, &az);
     -    secp256k1_fe_negate(&h, &u1, 1); secp256k1_fe_add(&h, &u2);
    -+    secp256k1_fe_negate(&h, &u1, 6); secp256k1_fe_add(&h, &u2);
    ++    secp256k1_fe_negate(&h, &u1, SECP256K1_GEJ_X_MAGNITUDE_MAX); secp256k1_fe_add(&h, &u2);
          secp256k1_fe_negate(&i, &s2, 1); secp256k1_fe_add(&i, &s1);
          if (secp256k1_fe_normalizes_to_zero_var(&h)) {
              if (secp256k1_fe_normalizes_to_zero_var(&i)) {
    @@ src/group_impl.h: static void secp256k1_gej_add_ge(secp256k1_gej *r, const secp2
      
          secp256k1_fe_sqr(&zz, &a->z);                       /* z = Z1^2 */
     -    u1 = a->x; secp256k1_fe_normalize_weak(&u1);        /* u1 = U1 = X1*Z2^2 (1) */
    -+    u1 = a->x;                                          /* u1 = U1 = X1*Z2^2 (6) */
    ++    u1 = a->x;                                          /* u1 = U1 = X1*Z2^2 (GEJ_X_M) */
          secp256k1_fe_mul(&u2, &b->x, &zz);                  /* u2 = U2 = X2*Z1^2 (1) */
     -    s1 = a->y; secp256k1_fe_normalize_weak(&s1);        /* s1 = S1 = Y1*Z2^3 (1) */
    -+    s1 = a->y;                                          /* s1 = S1 = Y1*Z2^3 (4) */
    ++    s1 = a->y;                                          /* s1 = S1 = Y1*Z2^3 (GEJ_Y_M) */
          secp256k1_fe_mul(&s2, &b->y, &zz);                  /* s2 = Y2*Z1^2 (1) */
          secp256k1_fe_mul(&s2, &s2, &a->z);                  /* s2 = S2 = Y2*Z1^3 (1) */
     -    t = u1; secp256k1_fe_add(&t, &u2);                  /* t = T = U1+U2 (2) */
     -    m = s1; secp256k1_fe_add(&m, &s2);                  /* m = M = S1+S2 (2) */
    -+    t = u1; secp256k1_fe_add(&t, &u2);                  /* t = T = U1+U2 (7) */
    -+    m = s1; secp256k1_fe_add(&m, &s2);                  /* m = M = S1+S2 (5) */
    ++    t = u1; secp256k1_fe_add(&t, &u2);                  /* t = T = U1+U2 (GEJ_X_M+1) */
    ++    m = s1; secp256k1_fe_add(&m, &s2);                  /* m = M = S1+S2 (GEJ_Y_M+1) */
          secp256k1_fe_sqr(&rr, &t);                          /* rr = T^2 (1) */
     -    secp256k1_fe_negate(&m_alt, &u2, 1);                /* Malt = -X2*Z1^2 */
     -    secp256k1_fe_mul(&tt, &u1, &m_alt);                 /* tt = -U1*U2 (2) */
    @@ src/group_impl.h: static void secp256k1_gej_add_ge(secp256k1_gej *r, const secp2
          rr_alt = s1;
     -    secp256k1_fe_mul_int(&rr_alt, 2);       /* rr = Y1*Z2^3 - Y2*Z1^3 (2) */
     -    secp256k1_fe_add(&m_alt, &u1);          /* Malt = X1*Z2^2 - X2*Z1^2 */
    -+    secp256k1_fe_mul_int(&rr_alt, 2);       /* rr_alt = Y1*Z2^3 - Y2*Z1^3 (8) */
    -+    secp256k1_fe_add(&m_alt, &u1);          /* Malt = X1*Z2^2 - X2*Z1^2 (8) */
    ++    secp256k1_fe_mul_int(&rr_alt, 2);       /* rr_alt = Y1*Z2^3 - Y2*Z1^3 (GEJ_Y_M*2) */
    ++    secp256k1_fe_add(&m_alt, &u1);          /* Malt = X1*Z2^2 - X2*Z1^2 (GEJ_X_M+2) */
      
     -    secp256k1_fe_cmov(&rr_alt, &rr, !degenerate);
     -    secp256k1_fe_cmov(&m_alt, &m, !degenerate);
    -+    secp256k1_fe_cmov(&rr_alt, &rr, !degenerate);       /* rr_alt (8) */
    -+    secp256k1_fe_cmov(&m_alt, &m, !degenerate);         /* m_alt (8) */
    ++    secp256k1_fe_cmov(&rr_alt, &rr, !degenerate);       /* rr_alt (GEJ_Y_M*2) */
    ++    secp256k1_fe_cmov(&m_alt, &m, !degenerate);         /* m_alt (GEJ_X_M+2) */
          /* Now Ralt / Malt = lambda and is guaranteed not to be Ralt / 0.
           * From here on out Ralt and Malt represent the numerator
           * and denominator of lambda; R and M represent the explicit
           * expressions x1^2 + x2^2 + x1x2 and y1 + y2. */
          secp256k1_fe_sqr(&n, &m_alt);                       /* n = Malt^2 (1) */
     -    secp256k1_fe_negate(&q, &t, 2);                     /* q = -T (3) */
    -+    secp256k1_fe_negate(&q, &t, 7);                     /* q = -T (8) */
    ++    secp256k1_fe_negate(&q, &t,
    ++        SECP256K1_GEJ_X_MAGNITUDE_MAX + 1);             /* q = -T (GEJ_X_M+2) */
          secp256k1_fe_mul(&q, &q, &n);                       /* q = Q = -T*Malt^2 (1) */
          /* These two lines use the observation that either M == Malt or M == 0,
           * so M^3 * Malt is either Malt^4 (which is computed by squaring), or
    @@ src/group_impl.h: static void secp256k1_gej_add_ge(secp256k1_gej *r, const secp2
     -    secp256k1_fe_sqr(&n, &n);
     -    secp256k1_fe_cmov(&n, &m, degenerate);              /* n = M^3 * Malt (2) */
     +    secp256k1_fe_sqr(&n, &n);                           /* n = Malt^4 (1) */
    -+    secp256k1_fe_cmov(&n, &m, degenerate);              /* n = M^3 * Malt (5) */
    ++    secp256k1_fe_cmov(&n, &m, degenerate);              /* n = M^3 * Malt (GEJ_Y_M+1) */
          secp256k1_fe_sqr(&t, &rr_alt);                      /* t = Ralt^2 (1) */
          secp256k1_fe_mul(&r->z, &a->z, &m_alt);             /* r->z = Z3 = Malt*Z (1) */
          secp256k1_fe_add(&t, &q);                           /* t = Ralt^2 + Q (2) */
    @@ src/group_impl.h: static void secp256k1_gej_add_ge(secp256k1_gej *r, const secp2
     -    secp256k1_fe_add(&t, &n);                           /* t = Ralt*(2*X3 + Q) + M^3*Malt (3) */
     -    secp256k1_fe_negate(&r->y, &t, 3);                  /* r->y = -(Ralt*(2*X3 + Q) + M^3*Malt) (4) */
     -    secp256k1_fe_half(&r->y);                           /* r->y = Y3 = -(Ralt*(2*X3 + Q) + M^3*Malt)/2 (3) */
    -+    secp256k1_fe_add(&t, &n);                           /* t = Ralt*(2*X3 + Q) + M^3*Malt (6) */
    -+    secp256k1_fe_negate(&r->y, &t, 6);                  /* r->y = -(Ralt*(2*X3 + Q) + M^3*Malt) (7) */
    -+    secp256k1_fe_half(&r->y);                           /* r->y = Y3 = -(Ralt*(2*X3 + Q) + M^3*Malt)/2 (4) */
    ++    secp256k1_fe_add(&t, &n);                           /* t = Ralt*(2*X3 + Q) + M^3*Malt (GEJ_Y_M+2) */
    ++    secp256k1_fe_negate(&r->y, &t,
    ++        SECP256K1_GEJ_Y_MAGNITUDE_MAX + 2);             /* r->y = -(Ralt*(2*X3 + Q) + M^3*Malt) (GEJ_Y_M+3) */
    ++    secp256k1_fe_half(&r->y);                           /* r->y = Y3 = -(Ralt*(2*X3 + Q) + M^3*Malt)/2 ((GEJ_Y_M+3)/2 + 1) */
      
          /* In case a->infinity == 1, replace r with (b->x, b->y, 1). */
          secp256k1_fe_cmov(&r->x, &b->x, a->infinity);

Any opinions on what of those changes should be adapted? On one hand I think it's nice to express the magnitudes in a generic way in the comments, on the other hand it probably causes more confusion and is not that helpful (especially for longer expressions). Also, is there any drawback in tightening the limits too much, e.g. increased risk that they have to be lifted again soon, causing extra work? Happy to receive input and change the PR either way.

[jonasnick]

@theStack I slightly prefer this variant and I don't think the resulting expressions are particularly confusing. For the code it makes more sense to use the SECP256K1_GE{,J}_{X,Y,Z}_MAGNITUDE_MAX constants instead of magic numbers. And then it's easier to follow the comments if they use the same constants.

[theStack]

Force-pushed the prepared variant which uses the magnitude limit constants both in the code and in the comments. Right now the tightest limits that still pass the tests are set ((4,3) for ge, (4,4,1) for gej), but this can be changed easily if there are concerns raised and more relaxed limits would be preferred (following the open discussion at #1348 (comment)).

[real-or-random]

ACK b7c685e

[jonasnick]

ACK b7c685e

[real-or-random]

@sipa Want to re-review this? If no, I think it's also ready for merge without another ACK.

[sipa]

utACK b7c685e