Repository: bitcoin-core/secp256k1 (c)

Issue:
Improve documentation for benchmarks
A follow-up PR could add a `--help` which would also mention the `get_iters` environment variable (I have to look up in the source how it's called exactly every time).

_Originally posted by @jonasnick in https://github.com/bitcoin-core/secp256k1/pull/991#pullrequestreview-799481944_

We could also remove the indirection in `cirrus.sh` (https://github.com/bitcoin-core/secp256k1/blob/master/ci/cirrus.sh#L35-L37) and define the "right" constants directly in `cirrus.yml`. I tend to look the constants up in `cirrus.yml` and then see the wrong constants without the `SECP256K1_` prefix.

Pull request description:
bench.c: add `--help` option and ci: move env variables
Fixes #1005

**Changes:**
- added `--help` option to `bench.c`
    - `help()` function prints the help to command-line
    - `have_invalid_args()` checks if the user has entered an invalid argument
- moved `secp256k1_bench_iters` and `secp256k1_test_iters` environment variables declaration to `.cirrus.yml`

Patch:
diff --git a/.cirrus.yml b/.cirrus.yml
index 82a0a99..99c8d04 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -19,9 +19,9 @@ env:
   RECOVERY: no
   SCHNORRSIG: no
   ### test options
-  TEST_ITERS:
+  SECP256K1_TEST_ITERS:
   BENCH: yes
-  BENCH_ITERS: 2
+  SECP256K1_BENCH_ITERS: 2
   CTIMETEST: yes
 
 cat_logs_snippet: &CAT_LOGS
@@ -171,7 +171,7 @@ task:
     memory: 1G
   env:
     WRAPPER_CMD: qemu-s390x
-    TEST_ITERS: 16
+    SECP256K1_TEST_ITERS: 16
     HOST: s390x-linux-gnu
     WITH_VALGRIND: no
     ECDH: yes
@@ -194,7 +194,7 @@ task:
     memory: 1G
   env:
     WRAPPER_CMD: qemu-arm
-    TEST_ITERS: 16
+    SECP256K1_TEST_ITERS: 16
     HOST: arm-linux-gnueabihf
     WITH_VALGRIND: no
     ECDH: yes
@@ -218,7 +218,7 @@ task:
     memory: 1G
   env:
     WRAPPER_CMD: qemu-aarch64
-    TEST_ITERS: 16
+    SECP256K1_TEST_ITERS: 16
     HOST: aarch64-linux-gnu
     WITH_VALGRIND: no
     ECDH: yes
@@ -239,7 +239,7 @@ task:
     memory: 1G
   env:
     WRAPPER_CMD: qemu-ppc64le
-    TEST_ITERS: 16
+    SECP256K1_TEST_ITERS: 16
     HOST: powerpc64le-linux-gnu
     WITH_VALGRIND: no
     ECDH: yes
@@ -260,7 +260,7 @@ task:
     memory: 1G
   env:
     WRAPPER_CMD: wine64-stable
-    TEST_ITERS: 16
+    SECP256K1_TEST_ITERS: 16
     HOST: x86_64-w64-mingw32
     WITH_VALGRIND: no
     ECDH: yes
@@ -290,15 +290,15 @@ task:
       env:
         # The `--error-exitcode` is required to make the test fail if valgrind found errors, otherwise it'll return 0 (https://www.valgrind.org/docs/manual/manual-core.html)
         WRAPPER_CMD: "valgrind --error-exitcode=42"
-        TEST_ITERS: 16
+        SECP256K1_TEST_ITERS: 16
     - name: "UBSan, ASan, LSan"
       env:
-        CFLAGS: "-fsanitize=undefined,address -g"
-        CFLAGS_FOR_BUILD: "-fsanitize=undefined,address -g"
+        CFLAGS: "-fsanitize=undefined,address"
+        CFLAGS_FOR_BUILD: "-fsanitize=undefined,address"
         UBSAN_OPTIONS: "print_stacktrace=1:halt_on_error=1"
         ASAN_OPTIONS: "strict_string_checks=1:detect_stack_use_after_return=1:detect_leaks=1"
         LSAN_OPTIONS: "use_unaligned=1"
-        TEST_ITERS: 32
+        SECP256K1_TEST_ITERS: 32
   # Try to cover many configurations with just a tiny matrix.
   matrix:
     - env:
@@ -329,7 +329,7 @@ task:
     # ./configure correctly errors out when given CC=g++.
     # We hack around this by passing CC=g++ only to make.
     CC: gcc
-    MAKEFLAGS: -j2 CC=g++ CFLAGS=-fpermissive\ -g
+    MAKEFLAGS: -j2 CC=g++ CFLAGS=-fpermissive
     WERROR_CFLAGS:
     EXPERIMENTAL: yes
     ECDH: yes
diff --git a/SECURITY.md b/SECURITY.md
index b515cc1..0e4d588 100644
--- a/SECURITY.md
+++ b/SECURITY.md
@@ -9,7 +9,7 @@ The following keys may be used to communicate sensitive information to developer
 | Name | Fingerprint |
 |------|-------------|
 | Pieter Wuille | 133E AC17 9436 F14A 5CF1  B794 860F EB80 4E66 9320 |
-| Jonas Nick | 36C7 1A37 C9D9 88BD E825  08D9 B1A7 0E4F 8DCD 0366 |
+| Andrew Poelstra | 699A 63EF C17A D3A9 A34C  FFC0 7AD0 A91C 40BD 0091 |
 | Tim Ruffing | 09E0 3F87 1092 E40E 106E  902B 33BC 86AB 80FF 5516 |
 
-You can import a key by running the following command with that individual’s fingerprint: `gpg --keyserver hkps://keys.openpgp.org --recv-keys "<fingerprint>"` Ensure that you put quotes around fingerprints containing spaces.
+You can import a key by running the following command with that individual’s fingerprint: `gpg --recv-keys "<fingerprint>"` Ensure that you put quotes around fingerprints containing spaces.
diff --git a/ci/cirrus.sh b/ci/cirrus.sh
index 39ec2ad..6809ab8 100755
--- a/ci/cirrus.sh
+++ b/ci/cirrus.sh
@@ -32,10 +32,6 @@ file .libs/* || true
 # This tells `make check` to wrap test invocations.
 export LOG_COMPILER="$WRAPPER_CMD"
 
-# This limits the iterations in the tests and benchmarks.
-export SECP256K1_TEST_ITERS="$TEST_ITERS"
-export SECP256K1_BENCH_ITERS="$BENCH_ITERS"
-
 make "$BUILD"
 
 if [ "$BENCH" = "yes" ]
diff --git a/configure.ac b/configure.ac
index 8727187..906f63c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -21,7 +21,7 @@ AC_PATH_TOOL(STRIP, strip)
 
 # Save definition of AC_PROG_CC because AM_PROG_CC_C_O in automake<=1.13 will
 # redefine AC_PROG_CC to exit with an error, which avoids the user calling it
-# accidentally and screwing up the effect of AM_PROG_CC_C_O. However, we'll need
+# accidently and screwing up the effect of AM_PROG_CC_C_O. However, we'll need
 # AC_PROG_CC later on in AX_PROG_CC_FOR_BUILD, where its usage is fine, and
 # we'll carefully make sure not to call AC_PROG_CC anywhere else.
 m4_copy([AC_PROG_CC], [saved_AC_PROG_CC])
@@ -43,8 +43,8 @@ case $host_os in
          # These Homebrew packages may be keg-only, meaning that they won't be found
          # in expected paths because they may conflict with system files. Ask
          # Homebrew where each one is located, then adjust paths accordingly.
-         if $BREW list --versions valgrind >/dev/null; then
-           valgrind_prefix=$($BREW --prefix valgrind 2>/dev/null)
+         valgrind_prefix=`$BREW --prefix valgrind 2>/dev/null`
+         if test x$valgrind_prefix != x; then
            VALGRIND_CPPFLAGS="-I$valgrind_prefix/include"
          fi
        else
@@ -160,7 +160,7 @@ AC_ARG_ENABLE(external_default_callbacks,
 AC_ARG_WITH([test-override-wide-multiply], [] ,[set_widemul=$withval], [set_widemul=auto])
 
 AC_ARG_WITH([asm], [AS_HELP_STRING([--with-asm=x86_64|arm|no|auto],
-[assembly optimizations to use (experimental: arm) [default=auto]])],[req_asm=$withval], [req_asm=auto])
+[assembly optimizations to use (experimental: arm) [default=auto]])],[req_asm=$withval], [req_asm=auto])
 
 AC_ARG_WITH([ecmult-window], [AS_HELP_STRING([--with-ecmult-window=SIZE|auto],
 [window size for ecmult precomputation for verification, specified as integer in range [2..24].]
diff --git a/doc/safegcd_implementation.md b/doc/safegcd_implementation.md
index 063aa8e..3ae556f 100644
--- a/doc/safegcd_implementation.md
+++ b/doc/safegcd_implementation.md
@@ -569,14 +569,8 @@ bits efficiently, which is possible on most platforms; it is abstracted here as
 
 ```python
 def count_trailing_zeros(v):
-    """
-    When v is zero, consider all N zero bits as "trailing".
-    For a non-zero value v, find z such that v=(d<<z) for some odd d.
-    """
-    if v == 0:
-        return N
-    else:
-        return (v & -v).bit_length() - 1
+    """For a non-zero value v, find z such that v=(d<<z) for some odd d."""
+    return (v & -v).bit_length() - 1
 
 i = N # divsteps left to do
 while True:
@@ -607,7 +601,7 @@ becomes negative, or when *i* reaches *0*. Combined, this is equivalent to addin
 It is easy to find what that multiple is: we want a number *w* such that *g+w&thinsp;f* has a few bottom
 zero bits. If that number of bits is *L*, we want *g+w&thinsp;f mod 2<sup>L</sup> = 0*, or *w = -g/f mod 2<sup>L</sup>*. Since *f*
 is odd, such a *w* exists for any *L*. *L* cannot be more than *i* steps (as we'd finish the loop before
-doing more) or more than *&eta;+1* steps (as we'd run `eta, f, g = -eta, g, -f` at that point), but
+doing more) or more than *&eta;+1* steps (as we'd run `eta, f, g = -eta, g, f` at that point), but
 apart from that, we're only limited by the complexity of computing *w*.
 
 This code demonstrates how to cancel up to 4 bits per step:
@@ -624,7 +618,7 @@ while True:
         break
     # We know g is odd now
     if eta < 0:
-        eta, f, g = -eta, g, -f
+        eta, f, g = -eta, g, f
     # Compute limit on number of bits to cancel
     limit = min(min(eta + 1, i), 4)
     # Compute w = -g/f mod 2**limit, using the table value for -1/f mod 2**4. Note that f is
diff --git a/include/secp256k1.h b/include/secp256k1.h
index 57114b8..576953f 100644
--- a/include/secp256k1.h
+++ b/include/secp256k1.h
@@ -264,7 +264,7 @@ SECP256K1_API void secp256k1_context_destroy(
  *  undefined.
  *
  *  When this function has not been called (or called with fn==NULL), then the
- *  default handler will be used. The library provides a default handler which
+ *  default handler will be used. The library provides a default handler which
  *  writes the message to stderr and calls abort. This default handler can be
  *  replaced at link time if the preprocessor macro
  *  USE_EXTERNAL_DEFAULT_CALLBACKS is defined, which is the case if the build
diff --git a/src/bench.c b/src/bench.c
index dfc0470..d5937b7 100644
--- a/src/bench.c
+++ b/src/bench.c
@@ -11,6 +11,51 @@
 #include "util.h"
 #include "bench.h"
 
+void help(int default_iters) {
+    printf("Benchmarks the following algorithms:\n");
+    printf("    - ECDSA signing/verification\n");
+
+#ifdef ENABLE_MODULE_ECDH
+    printf("    - ECDH key exchange (optional module)\n");
+#endif
+
+#ifdef ENABLE_MODULE_RECOVERY
+    printf("    - Public key recovery (optional module)\n");
+#endif
+
+#ifdef ENABLE_MODULE_SCHNORRSIG
+    printf("    - Schnorr signatures (optional module)\n");
+#endif
+
+    printf("\n");
+    printf("The default number of iterations for each benchmark is %d. This can be\n", default_iters);
+    printf("customized using the SECP256K1_BENCH_ITERS environment variable.\n");
+    printf("\n");
+    printf("Usage: ./bench [args]\n");
+    printf("By default, all benchmarks will be run.\n");
+    printf("args:\n");
+    printf("    help              : display this help and exit\n");
+    printf("    ecdsa             : all ECDSA algorithms--sign, verify, recovery (if enabled)\n");
+    printf("    ecdsa_sign        : ECDSA siging algorithm\n");
+    printf("    ecdsa_verify      : ECDSA verification algorithm\n");
+
+#ifdef ENABLE_MODULE_RECOVERY
+    printf("    ecdsa_recover     : ECDSA public key recovery algorithm\n");
+#endif
+
+#ifdef ENABLE_MODULE_ECDH
+    printf("    ecdh              : ECDH key exchange algorithm\n");
+#endif
+
+#ifdef ENABLE_MODULE_SCHNORRSIG
+    printf("    schnorrsig        : all Schnorr signature algorithms (sign, verify)\n");
+    printf("    schnorrsig_sign   : Schnorr sigining algorithm\n");
+    printf("    schnorrsig_verify : Schnorr verification algorithm\n");
+#endif
+
+    printf("\n");
+}
+
 typedef struct {
     secp256k1_context *ctx;
     unsigned char msg[32];
@@ -95,7 +140,52 @@ int main(int argc, char** argv) {
     bench_verify_data data;
 
     int d = argc == 1;
-    int iters = get_iters(20000);
+    int default_iters = 20000;
+    int iters = get_iters(default_iters);
+
+    /* Check for invalid user arguments */
+    char* valid_args[] = {"ecdsa", "verify", "ecdsa_verify", "sign", "ecdsa_sign", "ecdh", "recover",
+                         "ecdsa_recover", "schnorrsig", "schnorrsig_verify", "schnorrsig_sign"};
+    size_t valid_args_size = sizeof(valid_args)/sizeof(valid_args[0]);
+    int invalid_args = have_invalid_args(argc, argv, valid_args, valid_args_size);
+
+    if (argc > 1) {
+        if (have_flag(argc, argv, "-h")
+           || have_flag(argc, argv, "--help")
+           || have_flag(argc, argv, "help")) {
+            help(default_iters);
+            return 0;
+        } else if (invalid_args) {
+            fprintf(stderr, "./bench: unrecognized argument.\n\n");
+            help(default_iters);
+            return 1;
+        }
+    }
+
+/* Check if the user tries to benchmark optional module without building it */
+#ifndef ENABLE_MODULE_ECDH
+    if (have_flag(argc, argv, "ecdh")) { 
+        fprintf(stderr, "./bench: ECDH module not enabled.\n");
+        fprintf(stderr, "Use ./configure --enable-module-ecdh.\n\n");
+        return 1;
+    }
+#endif
+
+#ifndef ENABLE_MODULE_RECOVERY
+    if (have_flag(argc, argv, "recover") || have_flag(argc, argv, "ecdsa_recover")) { 
+        fprintf(stderr, "./bench: Public key recovery module not enabled.\n");
+        fprintf(stderr, "Use ./configure --enable-module-recovery.\n\n");
+        return 1;
+    }
+#endif
+
+#ifndef ENABLE_MODULE_SCHNORRSIG
+    if (have_flag(argc, argv, "schnorrsig") || have_flag(argc, argv, "schnorrsig_sign") || have_flag(argc, argv, "schnorrsig_verify")) { 
+        fprintf(stderr, "./bench: Schnorr signatures module not enabled.\n");
+        fprintf(stderr, "Use ./configure --enable-module-schnorrsig.\n\n");
+        return 1;
+    }
+#endif
 
     /* ECDSA verification benchmark */
     data.ctx = secp256k1_context_create(SECP256K1_CONTEXT_SIGN | SECP256K1_CONTEXT_VERIFY);
diff --git a/src/bench.h b/src/bench.h
index 65781a2..aa275fe 100644
--- a/src/bench.h
+++ b/src/bench.h
@@ -116,7 +116,7 @@ void run_benchmark(char *name, void (*benchmark)(void*, int), void (*setup)(void
 int have_flag(int argc, char** argv, char *flag) {
     char** argm = argv + argc;
     argv++;
-    while (argv != NULL && argv != argm) {
+    while (argv != argm) {
         if (strcmp(*argv, flag) == 0) {
             return 1;
         }
@@ -125,6 +125,32 @@ int have_flag(int argc, char** argv, char *flag) {
     return 0;
 }
 
+/* takes an array containing the arguments that the user is allowed to enter on the command-line
+   returns:
+      - 1 if the user entered an invalid argument
+      - 0 if all the user entered arguments are valid */
+int have_invalid_args(int argc, char** argv, char** valid_args, size_t n) {
+    size_t i;
+    int found_valid;
+    char** argm = argv + argc;
+    argv++;
+
+    while (argv != argm) {
+        found_valid = 0;
+        for (i = 0; i < n; i++) {
+            if (strcmp(*argv, valid_args[i]) == 0) {
+                found_valid = 1; /* user entered a valid arg from the list */
+                break;
+            }
+        }
+        if (found_valid == 0) {
+            return 1; /* invalid arg found */
+        }
+        argv++;
+    }
+    return 0;
+}
+
 int get_iters(int default_iters) {
     char* env = getenv("SECP256K1_BENCH_ITERS");
     if (env) {
diff --git a/src/bench_ecmult.c b/src/bench_ecmult.c
index 335d910..4030e02 100644
--- a/src/bench_ecmult.c
+++ b/src/bench_ecmult.c
@@ -292,7 +292,7 @@ int main(int argc, char **argv) {
            || have_flag(argc, argv, "--help")
            || have_flag(argc, argv, "help")) {
             help(argv);
-            return 1;
+            return 0;
         } else if(have_flag(argc, argv, "pippenger_wnaf")) {
             printf("Using pippenger_wnaf:\n");
             data.ecmult_multi = secp256k1_ecmult_pippenger_batch_single;
diff --git a/src/ecdsa_impl.h b/src/ecdsa_impl.h
index 90b4b22..a75a088 100644
--- a/src/ecdsa_impl.h
+++ b/src/ecdsa_impl.h
@@ -304,12 +304,12 @@ static int secp256k1_ecdsa_sig_sign(const secp256k1_ecmult_gen_context *ctx, sec
     high = secp256k1_scalar_is_high(sigs);
     secp256k1_scalar_cond_negate(sigs, high);
     if (recid) {
-        *recid ^= high;
+            *recid ^= high;
     }
     /* P.x = order is on the curve, so technically sig->r could end up being zero, which would be an invalid signature.
      * This is cryptographically unreachable as hitting it requires finding the discrete log of P.x = N.
      */
-    return (int)(!secp256k1_scalar_is_zero(sigr)) & (int)(!secp256k1_scalar_is_zero(sigs));
+    return !secp256k1_scalar_is_zero(sigr) & !secp256k1_scalar_is_zero(sigs);
 }
 
 #endif /* SECP256K1_ECDSA_IMPL_H */
diff --git a/src/ecmult_impl.h b/src/ecmult_impl.h
index 5bd4d4d..87ccc73 100644
--- a/src/ecmult_impl.h
+++ b/src/ecmult_impl.h
@@ -47,7 +47,7 @@
 
 /* The number of objects allocated on the scratch space for ecmult_multi algorithms */
 #define PIPPENGER_SCRATCH_OBJECTS 6
-#define STRAUSS_SCRATCH_OBJECTS 7
+#define STRAUSS_SCRATCH_OBJECTS 6
 
 #define PIPPENGER_MAX_BUCKET_WINDOW 12
 
@@ -220,7 +220,7 @@ struct secp256k1_strauss_state {
 static void secp256k1_ecmult_strauss_wnaf(const struct secp256k1_strauss_state *state, secp256k1_gej *r, size_t num, const secp256k1_gej *a, const secp256k1_scalar *na, const secp256k1_scalar *ng) {
     secp256k1_ge tmpa;
     secp256k1_fe Z;
-    /* Split G factors. */
+    /* Splitted G factors. */
     secp256k1_scalar ng_1, ng_128;
     int wnaf_ng_1[129];
     int bits_ng_1 = 0;
@@ -365,9 +365,6 @@ static int secp256k1_ecmult_strauss_batch(const secp256k1_callback* error_callba
         return 1;
     }
 
-    /* We allocate STRAUSS_SCRATCH_OBJECTS objects on the scratch space. If these
-     * allocations change, make sure to update the STRAUSS_SCRATCH_OBJECTS
-     * constant and strauss_scratch_size accordingly. */
     points = (secp256k1_gej*)secp256k1_scratch_alloc(error_callback, scratch, n_points * sizeof(secp256k1_gej));
     scalars = (secp256k1_scalar*)secp256k1_scratch_alloc(error_callback, scratch, n_points * sizeof(secp256k1_scalar));
     state.prej = (secp256k1_gej*)secp256k1_scratch_alloc(error_callback, scratch, n_points * ECMULT_TABLE_SIZE(WINDOW_A) * sizeof(secp256k1_gej));
@@ -662,12 +659,8 @@ static int secp256k1_ecmult_pippenger_batch(const secp256k1_callback* error_call
     if (inp_g_sc == NULL && n_points == 0) {
         return 1;
     }
-    bucket_window = secp256k1_pippenger_bucket_window(n_points);
 
-    /* We allocate PIPPENGER_SCRATCH_OBJECTS objects on the scratch space. If
-     * these allocations change, make sure to update the
-     * PIPPENGER_SCRATCH_OBJECTS constant and pippenger_scratch_size
-     * accordingly. */
+    bucket_window = secp256k1_pippenger_bucket_window(n_points);
     points = (secp256k1_ge *) secp256k1_scratch_alloc(error_callback, scratch, entries * sizeof(*points));
     scalars = (secp256k1_scalar *) secp256k1_scratch_alloc(error_callback, scratch, entries * sizeof(*scalars));
     state_space = (struct secp256k1_pippenger_state *) secp256k1_scratch_alloc(error_callback, scratch, sizeof(*state_space));
@@ -675,6 +668,7 @@ static int secp256k1_ecmult_pippenger_batch(const secp256k1_callback* error_call
         secp256k1_scratch_apply_checkpoint(error_callback, scratch, scratch_checkpoint);
         return 0;
     }
+
     state_space->ps = (struct secp256k1_pippenger_point_state *) secp256k1_scratch_alloc(error_callback, scratch, entries * sizeof(*state_space->ps));
     state_space->wnaf_na = (int *) secp256k1_scratch_alloc(error_callback, scratch, entries*(WNAF_SIZE(bucket_window+1)) * sizeof(int));
     buckets = (secp256k1_gej *) secp256k1_scratch_alloc(error_callback, scratch, (1<<bucket_window) * sizeof(*buckets));
diff --git a/src/scalar_impl.h b/src/scalar_impl.h
index 1b690e3..e124474 100644
--- a/src/scalar_impl.h
+++ b/src/scalar_impl.h
@@ -74,7 +74,7 @@ static void secp256k1_scalar_split_lambda_verify(const secp256k1_scalar *r1, con
  * Both lambda and beta are primitive cube roots of unity.  That is lamba^3 == 1 mod n and
  * beta^3 == 1 mod p, where n is the curve order and p is the field order.
  *
- * Furthermore, because (X^3 - 1) = (X - 1)(X^2 + X + 1), the primitive cube roots of unity are
+ * Futhermore, because (X^3 - 1) = (X - 1)(X^2 + X + 1), the primitive cube roots of unity are
  * roots of X^2 + X + 1.  Therefore lambda^2 + lamba == -1 mod n and beta^2 + beta == -1 mod p.
  * (The other primitive cube roots of unity are lambda^2 and beta^2 respectively.)
  *


Test patch:
diff --git a/src/tests.c b/src/tests.c
index 6027ebc..ebeef51 100644
--- a/src/tests.c
+++ b/src/tests.c
@@ -3225,7 +3225,7 @@ void test_intialized_inf(void) {
     secp256k1_gej pj, npj, infj1, infj2, infj3;
     secp256k1_fe zinv;
 
-    /* Test that adding P+(-P) results in a fully initialized infinity*/
+    /* Test that adding P+(-P) results in a fully initalized infinity*/
     random_group_element_test(&p);
     secp256k1_gej_set_ge(&pj, &p);
     secp256k1_gej_neg(&npj, &pj);