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

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

Pull request description:

  This addresses part of #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 31b4bbee1e115865a8a3aff6ccf04f6108371c5d
  stratospher:
    ACK 31b4bbe.

Tree-SHA512: 08bef9f63797cb8a1f3ea63c716c09aaa267dfee285b74ef5fbb47d614569d2787ec73d21bce080214872dfe70246f73cea42ad3c24e6baccecabe3312f71433
This commit is contained in:
Tim Ruffing 2023-05-19 09:54:40 +02:00
commit e9e4526a4e
No known key found for this signature in database
GPG Key ID: 8C461CCD293F6011
3 changed files with 19 additions and 17 deletions

View File

@ -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. * On input, both r and a must be valid field elements. Flag must be 0 or 1.
* Performs {r = flag ? a : r}. * 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); static void secp256k1_fe_cmov(secp256k1_fe *r, const secp256k1_fe *a, int flag);

View File

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

View File

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