Repository: bitcoin-core/secp256k1
Pull request #407 (2016-07-26 18:18:08)

Modify parameter order of internal functions to match API parameter order
The below commits modify the parameter orders for the following internal functions based on the API parameter ordering guide outlined in [secp256k1.h](https://github.com/bitcoin-core/secp256k1/blob/master/include/secp256k1.h#L10-L23).  This helps make the internal code's structure match the external API's structure.  These changes *shouldn't* harm any downstream applications that rely on this library as it only affects internal function parameter ordering, not external ones.

Each of the blow commits corresponds with modifying a single function's parameter order and updating all locations where it is used.

---

### secp256k1_fe_inv_all_var

    secp256k1_fe_inv_all_var(secp256k1_fe *r, const secp256k1_fe *a, size_t len)

**Declared in**: `src/field.h`
**Defined in**:  `src/field_impl.h`
**Used in**: `src/group_impl.h`, `src/tests.c`

- `size_t len` now comes after `const secp256k1_fe *a` as it defines its length

---

### ~~secp256k1_ge_globalz_set_table_gej~~

    secp256k1_ge_globalz_set_table_gej(secp256k1_ge *r, secp256k1_fe *globalz, const secp256k1_gej *a, const secp256k1_fe *zr, size_t len)

**Declared in**: `src/group.h`
**Defined in**: `src/group_impl.h`
**Used in**: `src/ecmult_impl.h`

- `size_t len` now comes after `const secp256k1_gej *a, const secp256k1_fe *zr` as it describes both of their lengths
- The API parameter ordering guide (rule 2) doesn't explicitly say what to do in this situation.
- Other options included:
  - `const secp256k1_gej *a, size_t len, const secp256k1_fe *zr` implying that `len` only describes `*a` and that `*zr` may be a single element.
  - `const secp256k1_gej *a, size_t a_len, const secp256k1_fe *zr, size_t zr_len` implying that `*a` and `*zr` are arrays, but possibly that `a_len != zr_len`.  This also means the caller must pass the same length value twice.

---

### secp256k1_ge_set_all_gej_var

    secp256k1_ge_set_all_gej_var(secp256k1_ge *r, const secp256k1_gej *a, size_t len, const secp256k1_callback *cb)

**Declared in**: `src/group.h`
**Defined in**: `src/group_impl.h`
**Used in**: `src/ecmult_gen_impl.h`, `src/tests.c`

- `size_t len` now comes after `const secp256k1_gej *a` as it describes its length "even if this violates rule 1"

---

### secp256k1_ge_set_table_gej_var

    secp256k1_ge_set_table_gej_var(secp256k1_ge *r, const secp256k1_gej *a, const secp256k1_fe *zr, size_t len)

**Declared in**: `src/group.h`
**Defined in**: `src/group_impl.h`
**Used in**: `src/ecmult_impl.h`, `src/tests.c`

- `size_t len` now comes last, same reasoning as `secp256k1_ge_globalz_set_table_gej`

---

### ~~secp256k1_ecmult_odd_multiples_table~~

    secp256k1_ecmult_odd_multiples_table(secp256k1_gej *prej, secp256k1_fe *zr, const secp256k1_gej *a, int n)

**Declared/Declared in**: `src/ecmult_impl.h`
**Used in**: `src/ecmult_impl.h`

- `int n` now comes last because it is a count parameter, not a length parameter
  - Length parameters are subject to rule 2, count parameters are subject to rule 4

---

### ~~secp256k1_ecmult_odd_multiples_table_storage_var~~

    secp256k1_ecmult_odd_multiples_table_storage_var(secp256k1_ge_storage *pre, const secp256k1_gej *a, const secp256k1_callback *cb, int n)

**Declared/Declared in**: `src/ecmult_impl.h`
**Used in**: `src/ecmult_impl.h`

- `int n` now comes last because it is a count parameter, not a length parameter
- It comes after `const secp256k1_callback *cb` because rule 4 states "Arguments that are not data pointers go last, from more complex to less complex: function pointers, ..., counts, ...".

---

I've compiled and run the included test utilities which reported that no errors were found.
However, I'd appreciate it if another developer reviewed these changes just in case I missed anything.

Cheers!


Patch:
diff --git a/src/ecmult_gen_impl.h b/src/ecmult_gen_impl.h
index b63c4d8..35f2546 100644
--- a/src/ecmult_gen_impl.h
+++ b/src/ecmult_gen_impl.h
@@ -77,7 +77,7 @@ static void secp256k1_ecmult_gen_context_build(secp256k1_ecmult_gen_context *ctx
                 secp256k1_gej_add_var(&numsbase, &numsbase, &nums_gej, NULL);
             }
         }
-        secp256k1_ge_set_all_gej_var(1024, prec, precj, cb);
+        secp256k1_ge_set_all_gej_var(prec, precj, 1024, cb);
     }
     for (j = 0; j < 64; j++) {
         for (i = 0; i < 16; i++) {
diff --git a/src/ecmult_impl.h b/src/ecmult_impl.h
index e6e5f47..f5e6513 100644
--- a/src/ecmult_impl.h
+++ b/src/ecmult_impl.h
@@ -101,7 +101,7 @@ static void secp256k1_ecmult_odd_multiples_table_storage_var(int n, secp256k1_ge
     /* Compute the odd multiples in Jacobian form. */
     secp256k1_ecmult_odd_multiples_table(n, prej, zr, a);
     /* Convert them in batch to affine coordinates. */
-    secp256k1_ge_set_table_gej_var(n, prea, prej, zr);
+    secp256k1_ge_set_table_gej_var(prea, prej, zr, n);
     /* Convert them to compact storage form. */
     for (i = 0; i < n; i++) {
         secp256k1_ge_to_storage(&pre[i], &prea[i]);
diff --git a/src/field.h b/src/field.h
index c5ba074..4daefc4 100644
--- a/src/field.h
+++ b/src/field.h
@@ -110,7 +110,7 @@ static void secp256k1_fe_inv_var(secp256k1_fe *r, const secp256k1_fe *a);
 /** Calculate the (modular) inverses of a batch of field elements. Requires the inputs' magnitudes to be
  *  at most 8. The output magnitudes are 1 (but not guaranteed to be normalized). The inputs and
  *  outputs must not overlap in memory. */
-static void secp256k1_fe_inv_all_var(size_t len, secp256k1_fe *r, const secp256k1_fe *a);
+static void secp256k1_fe_inv_all_var(secp256k1_fe *r, const secp256k1_fe *a, size_t len);
 
 /** Convert a field element to the storage type. */
 static void secp256k1_fe_to_storage(secp256k1_fe_storage *r, const secp256k1_fe *a);
diff --git a/src/field_impl.h b/src/field_impl.h
index 52cd902..5127b27 100644
--- a/src/field_impl.h
+++ b/src/field_impl.h
@@ -260,7 +260,7 @@ static void secp256k1_fe_inv_var(secp256k1_fe *r, const secp256k1_fe *a) {
 #endif
 }
 
-static void secp256k1_fe_inv_all_var(size_t len, secp256k1_fe *r, const secp256k1_fe *a) {
+static void secp256k1_fe_inv_all_var(secp256k1_fe *r, const secp256k1_fe *a, size_t len) {
     secp256k1_fe u;
     size_t i;
     if (len < 1) {
diff --git a/src/group.h b/src/group.h
index d515716..4957b24 100644
--- a/src/group.h
+++ b/src/group.h
@@ -65,12 +65,12 @@ static void secp256k1_ge_neg(secp256k1_ge *r, const secp256k1_ge *a);
 static void secp256k1_ge_set_gej(secp256k1_ge *r, secp256k1_gej *a);
 
 /** Set a batch of group elements equal to the inputs given in jacobian coordinates */
-static void secp256k1_ge_set_all_gej_var(size_t len, secp256k1_ge *r, const secp256k1_gej *a, const secp256k1_callback *cb);
+static void secp256k1_ge_set_all_gej_var(secp256k1_ge *r, const secp256k1_gej *a, size_t len, const secp256k1_callback *cb);
 
 /** Set a batch of group elements equal to the inputs given in jacobian
  *  coordinates (with known z-ratios). zr must contain the known z-ratios such
  *  that mul(a[i].z, zr[i+1]) == a[i+1].z. zr[0] is ignored. */
-static void secp256k1_ge_set_table_gej_var(size_t len, secp256k1_ge *r, const secp256k1_gej *a, const secp256k1_fe *zr);
+static void secp256k1_ge_set_table_gej_var(secp256k1_ge *r, const secp256k1_gej *a, const secp256k1_fe *zr, size_t len);
 
 /** Bring a batch inputs given in jacobian coordinates (with known z-ratios) to
  *  the same global z "denominator". zr must contain the known z-ratios such
diff --git a/src/group_impl.h b/src/group_impl.h
index 3e9c4c4..18f2b37 100644
--- a/src/group_impl.h
+++ b/src/group_impl.h
@@ -22,7 +22,7 @@ static const secp256k1_ge secp256k1_ge_const_g = SECP256K1_GE_CONST(
 );
 
 static void secp256k1_ge_set_gej_zinv(secp256k1_ge *r, const secp256k1_gej *a, const secp256k1_fe *zi) {
-    secp256k1_fe zi2; 
+    secp256k1_fe zi2;
     secp256k1_fe zi3;
     secp256k1_fe_sqr(&zi2, zi);
     secp256k1_fe_mul(&zi3, &zi2, zi);
@@ -76,7 +76,7 @@ static void secp256k1_ge_set_gej_var(secp256k1_ge *r, secp256k1_gej *a) {
     r->y = a->y;
 }
 
-static void secp256k1_ge_set_all_gej_var(size_t len, secp256k1_ge *r, const secp256k1_gej *a, const secp256k1_callback *cb) {
+static void secp256k1_ge_set_all_gej_var(secp256k1_ge *r, const secp256k1_gej *a, size_t len, const secp256k1_callback *cb) {
     secp256k1_fe *az;
     secp256k1_fe *azi;
     size_t i;
@@ -89,7 +89,7 @@ static void secp256k1_ge_set_all_gej_var(size_t len, secp256k1_ge *r, const secp
     }
 
     azi = (secp256k1_fe *)checked_malloc(cb, sizeof(secp256k1_fe) * count);
-    secp256k1_fe_inv_all_var(count, azi, az);
+    secp256k1_fe_inv_all_var(azi, az, count);
     free(az);
 
     count = 0;
@@ -102,7 +102,7 @@ static void secp256k1_ge_set_all_gej_var(size_t len, secp256k1_ge *r, const secp
     free(azi);
 }
 
-static void secp256k1_ge_set_table_gej_var(size_t len, secp256k1_ge *r, const secp256k1_gej *a, const secp256k1_fe *zr) {
+static void secp256k1_ge_set_table_gej_var(secp256k1_ge *r, const secp256k1_gej *a, const secp256k1_fe *zr, size_t len) {
     size_t i = len - 1;
     secp256k1_fe zi;
 
@@ -260,7 +260,7 @@ static void secp256k1_gej_double_var(secp256k1_gej *r, const secp256k1_gej *a, s
     /** For secp256k1, 2Q is infinity if and only if Q is infinity. This is because if 2Q = infinity,
      *  Q must equal -Q, or that Q.y == -(Q.y), or Q.y is 0. For a point on y^2 = x^3 + 7 to have
      *  y=0, x^3 must be -7 mod p. However, -7 has no cube root mod p.
-     *  
+     *
      *  Having said this, if this function receives a point on a sextic twist, e.g. by
      *  a fault attack, it is possible for y to be 0. This happens for y^2 = x^3 + 6,
      *  since -6 does have a cube root mod p. For this point, this function will not set


Test patch:
diff --git a/src/tests.c b/src/tests.c
index b32cb90..9ae7d30 100644
--- a/src/tests.c
+++ b/src/tests.c
@@ -520,7 +520,7 @@ void test_num_mod(void) {
     secp256k1_num order, n;
 
     /* check that 0 mod anything is 0 */
-    random_scalar_order_test(&s); 
+    random_scalar_order_test(&s);
     secp256k1_scalar_get_num(&order, &s);
     secp256k1_scalar_set_int(&s, 0);
     secp256k1_scalar_get_num(&n, &s);
@@ -535,7 +535,7 @@ void test_num_mod(void) {
     CHECK(secp256k1_num_is_zero(&n));
 
     /* check that increasing the number past 2^256 does not break this */
-    random_scalar_order_test(&s); 
+    random_scalar_order_test(&s);
     secp256k1_scalar_get_num(&n, &s);
     /* multiply by 2^8, which'll test this case with high probability */
     for (i = 0; i < 8; ++i) {
@@ -568,7 +568,7 @@ void test_num_jacobi(void) {
     /* we first need a scalar which is not a multiple of 5 */
     do {
         secp256k1_num fiven;
-        random_scalar_order_test(&sqr); 
+        random_scalar_order_test(&sqr);
         secp256k1_scalar_get_num(&fiven, &five);
         secp256k1_scalar_get_num(&n, &sqr);
         secp256k1_num_mod(&n, &fiven);
@@ -587,7 +587,7 @@ void test_num_jacobi(void) {
 
     /** test with secp group order as order */
     secp256k1_scalar_order_get_num(&order);
-    random_scalar_order_test(&sqr); 
+    random_scalar_order_test(&sqr);
     secp256k1_scalar_sqr(&sqr, &sqr);
     /* test residue */
     secp256k1_scalar_get_num(&n, &sqr);
@@ -1733,18 +1733,18 @@ void run_field_inv_all_var(void) {
     secp256k1_fe x[16], xi[16], xii[16];
     int i;
     /* Check it's safe to call for 0 elements */
-    secp256k1_fe_inv_all_var(0, xi, x);
+    secp256k1_fe_inv_all_var(xi, x, 0);
     for (i = 0; i < count; i++) {
         size_t j;
         size_t len = secp256k1_rand_int(15) + 1;
         for (j = 0; j < len; j++) {
             random_fe_non_zero(&x[j]);
         }
-        secp256k1_fe_inv_all_var(len, xi, x);
+        secp256k1_fe_inv_all_var(xi, x, len);
         for (j = 0; j < len; j++) {
             CHECK(check_fe_inverse(&x[j], &xi[j]));
         }
-        secp256k1_fe_inv_all_var(len, xii, xi);
+        secp256k1_fe_inv_all_var(xii, xi, len);
         for (j = 0; j < len; j++) {
             CHECK(check_fe_equal(&x[j], &xii[j]));
         }
@@ -1930,7 +1930,7 @@ void test_ge(void) {
                 zs[i] = gej[i].z;
             }
         }
-        secp256k1_fe_inv_all_var(4 * runs + 1, zinv, zs);
+        secp256k1_fe_inv_all_var(zinv, zs, 4 * runs + 1);
         free(zs);
     }
 
@@ -2050,8 +2050,8 @@ void test_ge(void) {
                 secp256k1_fe_mul(&zr[i + 1], &zinv[i], &gej[i + 1].z);
             }
         }
-        secp256k1_ge_set_table_gej_var(4 * runs + 1, ge_set_table, gej, zr);
-        secp256k1_ge_set_all_gej_var(4 * runs + 1, ge_set_all, gej, &ctx->error_callback);
+        secp256k1_ge_set_table_gej_var(ge_set_table, gej, zr, 4 * runs + 1);
+        secp256k1_ge_set_all_gej_var(ge_set_all, gej, 4 * runs + 1, &ctx->error_callback);
         for (i = 0; i < 4 * runs + 1; i++) {
             secp256k1_fe s;
             random_fe_non_zero(&s);