Merge bitcoin-core/secp256k1#1330: refactor: take use of secp256k1_scalar_{zero,one} constants

ade5b36 tests: add checks for scalar constants `secp256k1_scalar_{zero,one}` (Sebastian Falbesoner)
654246c refactor: take use of `secp256k1_scalar_{zero,one}` constants (Sebastian Falbesoner)

Pull request description:

  Rather than allocating a (non-constant) scalar variable on the stack with the sole purpose of setting it to a constant value, the global constants `secp256k1_scalar_{zero,one}` (apparently introduced in 34a67c7, PR bitcoin#710) can be directly used instead for the values 0 or 1. There is very likely not even a difference in run-time, but it leads to simpler and less code which might be nice.

ACKs for top commit:
  sipa:
    utACK ade5b36
  real-or-random:
    utACK ade5b36

Tree-SHA512: 0ff05a449c153f7117a4a56efef04b2087c2330f4692f3390a0b1d95573785ac7ae3fe689ed0ec2ecc64b575d2489d6e341d32567e75a1a4b4d458c3ecd406a1

Author: real-or-random

src/bench_ecmult.c

@@ -138,12 +138,10 @@ static void bench_ecmult_1p_teardown(void* arg, int iters) {
 
 static void bench_ecmult_0p_g(void* arg, int iters) {
     bench_data* data = (bench_data*)arg;
-    secp256k1_scalar zero;
     int i;
 
-    secp256k1_scalar_set_int(&zero, 0);
     for (i = 0; i < iters; ++i) {
-        secp256k1_ecmult(&data->output[i], NULL, &zero, &data->scalars[(data->offset1+i) % POINTS]);
+        secp256k1_ecmult(&data->output[i], NULL, &secp256k1_scalar_zero, &data->scalars[(data->offset1+i) % POINTS]);
     }
 }
 

src/eckey_impl.h

@@ -59,10 +59,8 @@ static int secp256k1_eckey_privkey_tweak_add(secp256k1_scalar *key, const secp25
 
 static int secp256k1_eckey_pubkey_tweak_add(secp256k1_ge *key, const secp256k1_scalar *tweak) {
     secp256k1_gej pt;
-    secp256k1_scalar one;
     secp256k1_gej_set_ge(&pt, key);
-    secp256k1_scalar_set_int(&one, 1);
-    secp256k1_ecmult(&pt, &pt, &one, tweak);
+    secp256k1_ecmult(&pt, &pt, &secp256k1_scalar_one, tweak);
 
     if (secp256k1_gej_is_infinity(&pt)) {
         return 0;
@@ -80,15 +78,13 @@ static int secp256k1_eckey_privkey_tweak_mul(secp256k1_scalar *key, const secp25
 }
 
 static int secp256k1_eckey_pubkey_tweak_mul(secp256k1_ge *key, const secp256k1_scalar *tweak) {
-    secp256k1_scalar zero;
     secp256k1_gej pt;
     if (secp256k1_scalar_is_zero(tweak)) {
         return 0;
     }
 
-    secp256k1_scalar_set_int(&zero, 0);
     secp256k1_gej_set_ge(&pt, key);
-    secp256k1_ecmult(&pt, &pt, tweak, &zero);
+    secp256k1_ecmult(&pt, &pt, tweak, &secp256k1_scalar_zero);
     secp256k1_ge_set_gej(key, &pt);
     return 1;
 }

src/ecmult_impl.h

@@ -770,14 +770,12 @@ static size_t secp256k1_pippenger_max_points(const secp256k1_callback* error_cal
  * require a scratch space */
 static int secp256k1_ecmult_multi_simple_var(secp256k1_gej *r, const secp256k1_scalar *inp_g_sc, secp256k1_ecmult_multi_callback cb, void *cbdata, size_t n_points) {
     size_t point_idx;
-    secp256k1_scalar szero;
     secp256k1_gej tmpj;
 
-    secp256k1_scalar_set_int(&szero, 0);
     secp256k1_gej_set_infinity(r);
     secp256k1_gej_set_infinity(&tmpj);
     /* r = inp_g_sc*G */
-    secp256k1_ecmult(r, &tmpj, &szero, inp_g_sc);
+    secp256k1_ecmult(r, &tmpj, &secp256k1_scalar_zero, inp_g_sc);
     for (point_idx = 0; point_idx < n_points; point_idx++) {
         secp256k1_ge point;
         secp256k1_gej pointj;
@@ -825,9 +823,7 @@ static int secp256k1_ecmult_multi_var(const secp256k1_callback* error_callback,
     if (inp_g_sc == NULL && n == 0) {
         return 1;
     } else if (n == 0) {
-        secp256k1_scalar szero;
-        secp256k1_scalar_set_int(&szero, 0);
-        secp256k1_ecmult(r, r, &szero, inp_g_sc);
+        secp256k1_ecmult(r, r, &secp256k1_scalar_zero, inp_g_sc);
         return 1;
     }
     if (scratch == NULL) {

src/tests.c

@@ -2307,26 +2307,23 @@ static void scalar_test(void) {
 
     {
         /* Test multiplicative identity. */
-        secp256k1_scalar r1, v1;
-        secp256k1_scalar_set_int(&v1,1);
-        secp256k1_scalar_mul(&r1, &s1, &v1);
+        secp256k1_scalar r1;
+        secp256k1_scalar_mul(&r1, &s1, &secp256k1_scalar_one);
         CHECK(secp256k1_scalar_eq(&r1, &s1));
     }
 
     {
         /* Test additive identity. */
-        secp256k1_scalar r1, v0;
-        secp256k1_scalar_set_int(&v0,0);
-        secp256k1_scalar_add(&r1, &s1, &v0);
+        secp256k1_scalar r1;
+        secp256k1_scalar_add(&r1, &s1, &secp256k1_scalar_zero);
         CHECK(secp256k1_scalar_eq(&r1, &s1));
     }
 
     {
         /* Test zero product property. */
-        secp256k1_scalar r1, v0;
-        secp256k1_scalar_set_int(&v0,0);
-        secp256k1_scalar_mul(&r1, &s1, &v0);
-        CHECK(secp256k1_scalar_eq(&r1, &v0));
+        secp256k1_scalar r1;
+        secp256k1_scalar_mul(&r1, &s1, &secp256k1_scalar_zero);
+        CHECK(secp256k1_scalar_eq(&r1, &secp256k1_scalar_zero));
     }
 
 }
@@ -2357,13 +2354,25 @@ static void run_scalar_tests(void) {
         run_scalar_set_b32_seckey_tests();
     }
 
+    {
+        /* Check that the scalar constants secp256k1_scalar_zero and
+           secp256k1_scalar_one contain the expected values. */
+        secp256k1_scalar zero, one;
+
+        CHECK(secp256k1_scalar_is_zero(&secp256k1_scalar_zero));
+        secp256k1_scalar_set_int(&zero, 0);
+        CHECK(secp256k1_scalar_eq(&zero, &secp256k1_scalar_zero));
+
+        CHECK(secp256k1_scalar_is_one(&secp256k1_scalar_one));
+        secp256k1_scalar_set_int(&one, 1);
+        CHECK(secp256k1_scalar_eq(&one, &secp256k1_scalar_one));
+    }
+
     {
         /* (-1)+1 should be zero. */
-        secp256k1_scalar s, o;
-        secp256k1_scalar_set_int(&s, 1);
-        CHECK(secp256k1_scalar_is_one(&s));
-        secp256k1_scalar_negate(&o, &s);
-        secp256k1_scalar_add(&o, &o, &s);
+        secp256k1_scalar o;
+        secp256k1_scalar_negate(&o, &secp256k1_scalar_one);
+        secp256k1_scalar_add(&o, &o, &secp256k1_scalar_one);
         CHECK(secp256k1_scalar_is_zero(&o));
         secp256k1_scalar_negate(&o, &o);
         CHECK(secp256k1_scalar_is_zero(&o));
@@ -2388,7 +2397,6 @@ static void run_scalar_tests(void) {
         secp256k1_scalar y;
         secp256k1_scalar z;
         secp256k1_scalar zz;
-        secp256k1_scalar one;
         secp256k1_scalar r1;
         secp256k1_scalar r2;
         secp256k1_scalar zzv;
@@ -2925,7 +2933,6 @@ static void run_scalar_tests(void) {
               0x1e, 0x86, 0x5d, 0x89, 0x63, 0xe6, 0x0a, 0x46,
               0x5c, 0x02, 0x97, 0x1b, 0x62, 0x43, 0x86, 0xf5}}
         };
-        secp256k1_scalar_set_int(&one, 1);
         for (i = 0; i < 33; i++) {
             secp256k1_scalar_set_b32(&x, chal[i][0], &overflow);
             CHECK(!overflow);
@@ -2948,7 +2955,7 @@ static void run_scalar_tests(void) {
                 CHECK(secp256k1_scalar_eq(&x, &z));
                 secp256k1_scalar_mul(&zz, &zz, &y);
                 CHECK(!secp256k1_scalar_check_overflow(&zz));
-                CHECK(secp256k1_scalar_eq(&one, &zz));
+                CHECK(secp256k1_scalar_eq(&secp256k1_scalar_one, &zz));
             }
         }
     }
@@ -4646,7 +4653,6 @@ static int ecmult_multi_false_callback(secp256k1_scalar *sc, secp256k1_ge *pt, s
 
 static void test_ecmult_multi(secp256k1_scratch *scratch, secp256k1_ecmult_multi_func ecmult_multi) {
     int ncount;
-    secp256k1_scalar szero;
     secp256k1_scalar sc[32];
     secp256k1_ge pt[32];
     secp256k1_gej r;
@@ -4655,7 +4661,6 @@ static void test_ecmult_multi(secp256k1_scratch *scratch, secp256k1_ecmult_multi
 
     data.sc = sc;
     data.pt = pt;
-    secp256k1_scalar_set_int(&szero, 0);
 
     /* No points to multiply */
     CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, NULL, ecmult_multi_callback, &data, 0));
@@ -4673,21 +4678,21 @@ static void test_ecmult_multi(secp256k1_scratch *scratch, secp256k1_ecmult_multi
         pt[1] = secp256k1_ge_const_g;
 
         /* only G scalar */
-        secp256k1_ecmult(&r2, &ptgj, &szero, &sc[0]);
+        secp256k1_ecmult(&r2, &ptgj, &secp256k1_scalar_zero, &sc[0]);
         CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &sc[0], ecmult_multi_callback, &data, 0));
         CHECK(secp256k1_gej_eq_var(&r, &r2));
 
         /* 1-point */
-        secp256k1_ecmult(&r2, &ptgj, &sc[0], &szero);
-        CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &szero, ecmult_multi_callback, &data, 1));
+        secp256k1_ecmult(&r2, &ptgj, &sc[0], &secp256k1_scalar_zero);
+        CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &secp256k1_scalar_zero, ecmult_multi_callback, &data, 1));
         CHECK(secp256k1_gej_eq_var(&r, &r2));
 
         /* Try to multiply 1 point, but callback returns false */
-        CHECK(!ecmult_multi(&CTX->error_callback, scratch, &r, &szero, ecmult_multi_false_callback, &data, 1));
+        CHECK(!ecmult_multi(&CTX->error_callback, scratch, &r, &secp256k1_scalar_zero, ecmult_multi_false_callback, &data, 1));
 
         /* 2-point */
         secp256k1_ecmult(&r2, &ptgj, &sc[0], &sc[1]);
-        CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &szero, ecmult_multi_callback, &data, 2));
+        CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &secp256k1_scalar_zero, ecmult_multi_callback, &data, 2));
         CHECK(secp256k1_gej_eq_var(&r, &r2));
 
         /* 2-point with G scalar */
@@ -4707,7 +4712,7 @@ static void test_ecmult_multi(secp256k1_scratch *scratch, secp256k1_ecmult_multi
                 random_scalar_order(&sc[i]);
                 secp256k1_ge_set_infinity(&pt[i]);
             }
-            CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &szero, ecmult_multi_callback, &data, sizes[j]));
+            CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &secp256k1_scalar_zero, ecmult_multi_callback, &data, sizes[j]));
             CHECK(secp256k1_gej_is_infinity(&r));
         }
 
@@ -4717,7 +4722,7 @@ static void test_ecmult_multi(secp256k1_scratch *scratch, secp256k1_ecmult_multi
                 pt[i] = ptg;
                 secp256k1_scalar_set_int(&sc[i], 0);
             }
-            CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &szero, ecmult_multi_callback, &data, sizes[j]));
+            CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &secp256k1_scalar_zero, ecmult_multi_callback, &data, sizes[j]));
             CHECK(secp256k1_gej_is_infinity(&r));
         }
 
@@ -4730,7 +4735,7 @@ static void test_ecmult_multi(secp256k1_scratch *scratch, secp256k1_ecmult_multi
                 pt[2 * i + 1] = ptg;
             }
 
-            CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &szero, ecmult_multi_callback, &data, sizes[j]));
+            CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &secp256k1_scalar_zero, ecmult_multi_callback, &data, sizes[j]));
             CHECK(secp256k1_gej_is_infinity(&r));
 
             random_scalar_order(&sc[0]);
@@ -4743,7 +4748,7 @@ static void test_ecmult_multi(secp256k1_scratch *scratch, secp256k1_ecmult_multi
                 secp256k1_ge_neg(&pt[2*i+1], &pt[2*i]);
             }
 
-            CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &szero, ecmult_multi_callback, &data, sizes[j]));
+            CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &secp256k1_scalar_zero, ecmult_multi_callback, &data, sizes[j]));
             CHECK(secp256k1_gej_is_infinity(&r));
         }
 
@@ -4758,7 +4763,7 @@ static void test_ecmult_multi(secp256k1_scratch *scratch, secp256k1_ecmult_multi
             secp256k1_scalar_negate(&sc[i], &sc[i]);
         }
 
-        CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &szero, ecmult_multi_callback, &data, 32));
+        CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &secp256k1_scalar_zero, ecmult_multi_callback, &data, 32));
         CHECK(secp256k1_gej_is_infinity(&r));
     }
 
@@ -4776,8 +4781,8 @@ static void test_ecmult_multi(secp256k1_scratch *scratch, secp256k1_ecmult_multi
             secp256k1_gej_add_ge_var(&r, &r, &pt[i], NULL);
         }
 
-        secp256k1_ecmult(&r2, &r, &sc[0], &szero);
-        CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &szero, ecmult_multi_callback, &data, 20));
+        secp256k1_ecmult(&r2, &r, &sc[0], &secp256k1_scalar_zero);
+        CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &secp256k1_scalar_zero, ecmult_multi_callback, &data, 20));
         CHECK(secp256k1_gej_eq_var(&r, &r2));
     }
 
@@ -4797,8 +4802,8 @@ static void test_ecmult_multi(secp256k1_scratch *scratch, secp256k1_ecmult_multi
         }
 
         secp256k1_gej_set_ge(&p0j, &pt[0]);
-        secp256k1_ecmult(&r2, &p0j, &rs, &szero);
-        CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &szero, ecmult_multi_callback, &data, 20));
+        secp256k1_ecmult(&r2, &p0j, &rs, &secp256k1_scalar_zero);
+        CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &secp256k1_scalar_zero, ecmult_multi_callback, &data, 20));
         CHECK(secp256k1_gej_eq_var(&r, &r2));
     }
 
@@ -4809,13 +4814,13 @@ static void test_ecmult_multi(secp256k1_scratch *scratch, secp256k1_ecmult_multi
     }
 
     secp256k1_scalar_clear(&sc[0]);
-    CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &szero, ecmult_multi_callback, &data, 20));
+    CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &secp256k1_scalar_zero, ecmult_multi_callback, &data, 20));
     secp256k1_scalar_clear(&sc[1]);
     secp256k1_scalar_clear(&sc[2]);
     secp256k1_scalar_clear(&sc[3]);
     secp256k1_scalar_clear(&sc[4]);
-    CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &szero, ecmult_multi_callback, &data, 6));
-    CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &szero, ecmult_multi_callback, &data, 5));
+    CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &secp256k1_scalar_zero, ecmult_multi_callback, &data, 6));
+    CHECK(ecmult_multi(&CTX->error_callback, scratch, &r, &secp256k1_scalar_zero, ecmult_multi_callback, &data, 5));
     CHECK(secp256k1_gej_is_infinity(&r));
 
     /* Run through s0*(t0*P) + s1*(t1*P) exhaustively for many small values of s0, s1, t0, t1 */
@@ -4839,8 +4844,8 @@ static void test_ecmult_multi(secp256k1_scratch *scratch, secp256k1_ecmult_multi
                 secp256k1_scalar_set_int(&t1, (t1i + 1) / 2);
                 secp256k1_scalar_cond_negate(&t1, t1i & 1);
 
-                secp256k1_ecmult(&t0p, &ptgj, &t0, &szero);
-                secp256k1_ecmult(&t1p, &ptgj, &t1, &szero);
+                secp256k1_ecmult(&t0p, &ptgj, &t0, &secp256k1_scalar_zero);
+                secp256k1_ecmult(&t1p, &ptgj, &t1, &secp256k1_scalar_zero);
 
                 for(s0i = 0; s0i < TOP; s0i++) {
                     for(s1i = 0; s1i < TOP; s1i++) {
@@ -4859,8 +4864,8 @@ static void test_ecmult_multi(secp256k1_scratch *scratch, secp256k1_ecmult_multi
                         secp256k1_scalar_mul(&tmp2, &t1, &sc[1]);
                         secp256k1_scalar_add(&tmp1, &tmp1, &tmp2);
 
-                        secp256k1_ecmult(&expected, &ptgj, &tmp1, &szero);
-                        CHECK(ecmult_multi(&CTX->error_callback, scratch, &actual, &szero, ecmult_multi_callback, &data, 2));
+                        secp256k1_ecmult(&expected, &ptgj, &tmp1, &secp256k1_scalar_zero);
+                        CHECK(ecmult_multi(&CTX->error_callback, scratch, &actual, &secp256k1_scalar_zero, ecmult_multi_callback, &data, 2));
                         CHECK(secp256k1_gej_eq_var(&actual, &expected));
                     }
                 }
@@ -5036,7 +5041,6 @@ static int test_ecmult_multi_random(secp256k1_scratch *scratch) {
 }
 
 static void test_ecmult_multi_batch_single(secp256k1_ecmult_multi_func ecmult_multi) {
-    secp256k1_scalar szero;
     secp256k1_scalar sc;
     secp256k1_ge pt;
     secp256k1_gej r;
@@ -5047,11 +5051,10 @@ static void test_ecmult_multi_batch_single(secp256k1_ecmult_multi_func ecmult_mu
     random_scalar_order(&sc);
     data.sc = &sc;
     data.pt = &pt;
-    secp256k1_scalar_set_int(&szero, 0);
 
     /* Try to multiply 1 point, but scratch space is empty.*/
     scratch_empty = secp256k1_scratch_create(&CTX->error_callback, 0);
-    CHECK(!ecmult_multi(&CTX->error_callback, scratch_empty, &r, &szero, ecmult_multi_callback, &data, 1));
+    CHECK(!ecmult_multi(&CTX->error_callback, scratch_empty, &r, &secp256k1_scalar_zero, ecmult_multi_callback, &data, 1));
     secp256k1_scratch_destroy(&CTX->error_callback, scratch_empty);
 }
 
@@ -5159,7 +5162,6 @@ static void test_ecmult_multi_batch_size_helper(void) {
 static void test_ecmult_multi_batching(void) {
     static const int n_points = 2*ECMULT_PIPPENGER_THRESHOLD;
     secp256k1_scalar scG;
-    secp256k1_scalar szero;
     secp256k1_scalar *sc = (secp256k1_scalar *)checked_malloc(&CTX->error_callback, sizeof(secp256k1_scalar) * n_points);
     secp256k1_ge *pt = (secp256k1_ge *)checked_malloc(&CTX->error_callback, sizeof(secp256k1_ge) * n_points);
     secp256k1_gej r;
@@ -5169,11 +5171,10 @@ static void test_ecmult_multi_batching(void) {
     secp256k1_scratch *scratch;
 
     secp256k1_gej_set_infinity(&r2);
-    secp256k1_scalar_set_int(&szero, 0);
 
     /* Get random scalars and group elements and compute result */
     random_scalar_order(&scG);
-    secp256k1_ecmult(&r2, &r2, &szero, &scG);
+    secp256k1_ecmult(&r2, &r2, &secp256k1_scalar_zero, &scG);
     for(i = 0; i < n_points; i++) {
         secp256k1_ge ptg;
         secp256k1_gej ptgj;