Merge #1171: Change ARG_CHECK_NO_RETURN to ARG_CHECK_VOID which returns (void)

a49e094 docs: Fix typo (Tim Ruffing)
2551cda tests: Fix code formatting (Tim Ruffing)
c635c1b Change ARG_CHECK_NO_RETURN to ARG_CHECK_VOID which returns (void) (Tim Ruffing)
cf66f23 refactor: Add helper function secp256k1_context_is_proper() (Tim Ruffing)

Pull request description:

ACKs for top commit:
  sipa:
    utACK a49e094
  jonasnick:
    ACK a49e094

Tree-SHA512: 0fd4ee88510f2de0de96378ae69ce6e610a446000bb78597026c5924803e1ce5a4f76303fc6446233a6129f9c42dce1b1549f93bef935131101e47b5a69cdf2f

Author: jonasnick

include/secp256k1.h

@@ -849,7 +849,7 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_pubkey_tweak_mul(
  * kind of elliptic curve point multiplication and thus does not benefit from
  * enhanced protection against side-channel leakage currently.
  *
- * It is safe call this function on a copy of secp256k1_context_static in writable
+ * It is safe to call this function on a copy of secp256k1_context_static in writable
  * memory (e.g., obtained via secp256k1_context_clone). In that case, this
  * function is guaranteed to return 1, but the call will have no effect because
  * the static context (or a copy thereof) is not meant to be randomized.

src/secp256k1.c

@@ -51,9 +51,10 @@
     } \
 } while(0)
 
-#define ARG_CHECK_NO_RETURN(cond) do { \
+#define ARG_CHECK_VOID(cond) do { \
     if (EXPECT(!(cond), 0)) { \
         secp256k1_callback_call(&ctx->illegal_callback, #cond); \
+        return; \
     } \
 } while(0)
 
@@ -75,6 +76,15 @@ static const secp256k1_context secp256k1_context_static_ = {
 const secp256k1_context *secp256k1_context_static = &secp256k1_context_static_;
 const secp256k1_context *secp256k1_context_no_precomp = &secp256k1_context_static_;
 
+/* Helper function that determines if a context is proper, i.e., is not the static context or a copy thereof.
+ *
+ * This is intended for "context" functions such as secp256k1_context_clone. Function which need specific
+ * features of a context should still check for these features directly. For example, a function that needs
+ * ecmult_gen should directly check for the existence of the ecmult_gen context. */
+static int secp256k1_context_is_proper(const secp256k1_context* ctx) {
+    return secp256k1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx);
+}
+
 void secp256k1_selftest(void) {
     if (!secp256k1_selftest_passes()) {
         secp256k1_callback_call(&default_error_callback, "self test failed");
@@ -157,7 +167,7 @@ secp256k1_context* secp256k1_context_clone(const secp256k1_context* ctx) {
 }
 
 void secp256k1_context_preallocated_destroy(secp256k1_context* ctx) {
-    ARG_CHECK_NO_RETURN(ctx != secp256k1_context_static);
+    ARG_CHECK_VOID(ctx != secp256k1_context_static);
     if (ctx != NULL) {
         secp256k1_ecmult_gen_context_clear(&ctx->ecmult_gen_ctx);
     }
@@ -171,7 +181,10 @@ void secp256k1_context_destroy(secp256k1_context* ctx) {
 }
 
 void secp256k1_context_set_illegal_callback(secp256k1_context* ctx, void (*fun)(const char* message, void* data), const void* data) {
-    ARG_CHECK_NO_RETURN(ctx != secp256k1_context_static);
+    /* We compare pointers instead of checking secp256k1_context_is_proper() here
+       because setting callbacks is allowed on *copies* of the static context:
+       it's harmless and makes testing easier. */
+    ARG_CHECK_VOID(ctx != secp256k1_context_static);
     if (fun == NULL) {
         fun = secp256k1_default_illegal_callback_fn;
     }
@@ -180,7 +193,10 @@ void secp256k1_context_set_illegal_callback(secp256k1_context* ctx, void (*fun)(
 }
 
 void secp256k1_context_set_error_callback(secp256k1_context* ctx, void (*fun)(const char* message, void* data), const void* data) {
-    ARG_CHECK_NO_RETURN(ctx != secp256k1_context_static);
+    /* We compare pointers instead of checking secp256k1_context_is_proper() here
+       because setting callbacks is allowed on *copies* of the static context:
+       it's harmless and makes testing easier. */
+    ARG_CHECK_VOID(ctx != secp256k1_context_static);
     if (fun == NULL) {
         fun = secp256k1_default_error_callback_fn;
     }

src/tests.c

@@ -152,8 +152,7 @@ int context_eq(const secp256k1_context *a, const secp256k1_context *b) {
     return a->declassify == b->declassify
             && ecmult_gen_context_eq(&a->ecmult_gen_ctx, &b->ecmult_gen_ctx)
             && a->illegal_callback.fn == b->illegal_callback.fn
-            && a->illegal_callback.data == b->illegal_callback.
-data
+            && a->illegal_callback.data == b->illegal_callback.data
             && a->error_callback.fn == b->error_callback.fn
             && a->error_callback.data == b->error_callback.data;
 }