Merge bitcoin-core/secp256k1#1089: Schnorrsig API improvements

b8f8b99 docs: Fix return value for functions that don't have invalid inputs (Tim Ruffing)
f813bb0 schnorrsig: Adapt example to new API (Tim Ruffing)
99e6568 schnorrsig: Rename schnorrsig_sign to schnorsig_sign32 and deprecate (Tim Ruffing)
fc94a2d Use SECP256K1_DEPRECATED for existing deprecated API functions (Tim Ruffing)
3db0560 Add SECP256K1_DEPRECATED attribute for marking API parts as deprecated (Tim Ruffing)

Pull request description:

  Should be merged before bitcoin#995 if we want this.

  I suspect the only change here which is debatable on a conceptual level is the renaming. I can drop this of course.

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

Tree-SHA512: 7c5b9715013002eecbf2e649032673204f6eaffe156f20e3ddf51fab938643847d23068f11b127ef3d7fe759e42a20ecaf2ec98718d901ef9eaadbc9853c1dfe

examples/schnorr.c

@@ -18,16 +18,9 @@
 #include "random.h"
 
 int main(void) {
-    /* Instead of signing the message directly, we must sign a 32-byte hash.
-     * Here the message is "Hello, world!" and the hash function was SHA-256.
-     * An actual implementation should just call SHA-256, but this example
-     * hardcodes the output to avoid depending on an additional library. */
-    unsigned char msg_hash[32] = {
-        0x31, 0x5F, 0x5B, 0xDB, 0x76, 0xD0, 0x78, 0xC4,
-        0x3B, 0x8A, 0xC0, 0x06, 0x4E, 0x4A, 0x01, 0x64,
-        0x61, 0x2B, 0x1F, 0xCE, 0x77, 0xC8, 0x69, 0x34,
-        0x5B, 0xFC, 0x94, 0xC7, 0x58, 0x94, 0xED, 0xD3,
-    };
+    unsigned char msg[12] = "Hello World!";
+    unsigned char msg_hash[32];
+    unsigned char tag[17] = "my_fancy_protocol";
     unsigned char seckey[32];
     unsigned char randomize[32];
     unsigned char auxiliary_rand[32];
@@ -84,19 +77,38 @@ int main(void) {
 
     /*** Signing ***/
 
+    /* Instead of signing (possibly very long) messages directly, we sign a
+     * 32-byte hash of the message in this example.
+     *
+     * We use secp256k1_tagged_sha256 to create this hash. This function expects
+     * a context-specific "tag", which restricts the context in which the signed
+     * messages should be considered valid. For example, if protocol A mandates
+     * to use the tag "my_fancy_protocol" and protocol B mandates to use the tag
+     * "my_boring_protocol", then signed messages from protocol A will never be
+     * valid in protocol B (and vice versa), even if keys are reused across
+     * protocols. This implements "domain separation", which is considered good
+     * practice. It avoids attacks in which users are tricked into signing a
+     * message that has intended consequences in the intended context (e.g.,
+     * protocol A) but would have unintended consequences if it were valid in
+     * some other context (e.g., protocol B). */
+    return_val = secp256k1_tagged_sha256(ctx, msg_hash, tag, sizeof(tag), msg, sizeof(msg));
+    assert(return_val);
+
     /* Generate 32 bytes of randomness to use with BIP-340 schnorr signing. */
     if (!fill_random(auxiliary_rand, sizeof(auxiliary_rand))) {
         printf("Failed to generate randomness\n");
         return 1;
     }
 
-    /* Generate a Schnorr signature `noncefp` and `ndata` allows you to pass a
-     * custom nonce function, passing `NULL` will use the BIP-340 safe default.
-     * BIP-340 recommends passing 32 bytes of randomness to the nonce function to
-     * improve security against side-channel attacks. Signing with a valid
-     * context, verified keypair and the default nonce function should never
-     * fail. */
-    return_val = secp256k1_schnorrsig_sign(ctx, signature, msg_hash, &keypair, auxiliary_rand);
+    /* Generate a Schnorr signature.
+     *
+     * We use the secp256k1_schnorrsig_sign32 function that provides a simple
+     * interface for signing 32-byte messages (which in our case is a hash of
+     * the actual message). BIP-340 recommends passing 32 bytes of randomness
+     * to the signing function to improve security against side-channel attacks.
+     * Signing with a valid context, a 32-byte message, a verified keypair, and
+     * any 32 bytes of auxiliary random data should never fail. */
+    return_val = secp256k1_schnorrsig_sign32(ctx, signature, msg_hash, &keypair, auxiliary_rand);
     assert(return_val);
 
     /*** Verification ***/
@@ -108,6 +120,10 @@ int main(void) {
         return 1;
     }
 
+    /* Compute the tagged hash on the received messages using the same tag as the signer. */
+    return_val = secp256k1_tagged_sha256(ctx, msg_hash, tag, sizeof(tag), msg, sizeof(msg));
+    assert(return_val);
+
     /* Verify a signature. This will return 1 if it's valid and 0 if it's not. */
     is_signature_valid = secp256k1_schnorrsig_verify(ctx, signature, msg_hash, 32, &pubkey);
 

include/secp256k1.h

@@ -169,6 +169,17 @@ typedef int (*secp256k1_nonce_function)(
 #  define SECP256K1_ARG_NONNULL(_x)
 # endif
 
+/** Attribute for marking functions, types, and variables as deprecated */
+#if !defined(SECP256K1_BUILD) && defined(__has_attribute)
+# if __has_attribute(__deprecated__)
+#  define SECP256K1_DEPRECATED(_msg) __attribute__ ((__deprecated__(_msg)))
+# else
+#  define SECP256K1_DEPRECATED(_msg)
+# endif
+#else
+# define SECP256K1_DEPRECATED(_msg)
+#endif
+
 /** All flags' lower 8 bits indicate what they're for. Do not use directly. */
 #define SECP256K1_FLAGS_TYPE_MASK ((1 << 8) - 1)
 #define SECP256K1_FLAGS_TYPE_CONTEXT (1 << 0)
@@ -641,7 +652,8 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_seckey_negate(
 SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_privkey_negate(
     const secp256k1_context* ctx,
     unsigned char *seckey
-) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2);
+) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2)
+  SECP256K1_DEPRECATED("Use secp256k1_ec_seckey_negate instead");
 
 /** Negates a public key in place.
  *
@@ -681,7 +693,8 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_privkey_tweak_add(
     const secp256k1_context* ctx,
     unsigned char *seckey,
     const unsigned char *tweak32
-) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3);
+) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3)
+  SECP256K1_DEPRECATED("Use secp256k1_ec_seckey_tweak_add instead");
 
 /** Tweak a public key by adding tweak times the generator to it.
  *
@@ -727,7 +740,8 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_privkey_tweak_mul(
     const secp256k1_context* ctx,
     unsigned char *seckey,
     const unsigned char *tweak32
-) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3);
+) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3)
+  SECP256K1_DEPRECATED("Use secp256k1_ec_seckey_tweak_mul instead");
 
 /** Tweak a public key by multiplying it by a tweak value.
  *
@@ -800,7 +814,7 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_pubkey_combine(
  *  implementations optimized for a specific tag can precompute the SHA256 state
  *  after hashing the tag hashes.
  *
- *  Returns 0 if the arguments are invalid and 1 otherwise.
+ *  Returns: 1 always.
  *  Args:    ctx: pointer to a context object
  *  Out:  hash32: pointer to a 32-byte array to store the resulting hash
  *  In:      tag: pointer to an array containing the tag

include/secp256k1_extrakeys.h

@@ -81,8 +81,7 @@ SECP256K1_API int secp256k1_xonly_pubkey_cmp(
 
 /** Converts a secp256k1_pubkey into a secp256k1_xonly_pubkey.
  *
- *  Returns: 1 if the public key was successfully converted
- *           0 otherwise
+ *  Returns: 1 always.
  *
  *  Args:         ctx: pointer to a context object.
  *  Out: xonly_pubkey: pointer to an x-only public key object for placing the converted public key.
@@ -172,7 +171,7 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_keypair_create(
 
 /** Get the secret key from a keypair.
  *
- *  Returns: 0 if the arguments are invalid. 1 otherwise.
+ *  Returns: 1 always.
  *  Args:   ctx: pointer to a context object.
  *  Out: seckey: pointer to a 32-byte buffer for the secret key.
  *  In: keypair: pointer to a keypair.
@@ -185,7 +184,7 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_keypair_sec(
 
 /** Get the public key from a keypair.
  *
- *  Returns: 0 if the arguments are invalid. 1 otherwise.
+ *  Returns: 1 always.
  *  Args:    ctx: pointer to a context object.
  *  Out: pubkey: pointer to a pubkey object. If 1 is returned, it is set to
  *               the keypair public key. If not, it's set to an invalid value.
@@ -202,7 +201,7 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_keypair_pub(
  *  This is the same as calling secp256k1_keypair_pub and then
  *  secp256k1_xonly_pubkey_from_pubkey.
  *
- *  Returns: 0 if the arguments are invalid. 1 otherwise.
+ *  Returns: 1 always.
  *  Args:   ctx: pointer to a context object.
  *  Out: pubkey: pointer to an xonly_pubkey object. If 1 is returned, it is set
  *               to the keypair public key after converting it to an

include/secp256k1_schnorrsig.h

@@ -116,14 +116,25 @@ typedef struct {
  *                BIP-340 "Default Signing" for a full explanation of this
  *                argument and for guidance if randomness is expensive.
  */
-SECP256K1_API int secp256k1_schnorrsig_sign(
+SECP256K1_API int secp256k1_schnorrsig_sign32(
     const secp256k1_context* ctx,
     unsigned char *sig64,
     const unsigned char *msg32,
     const secp256k1_keypair *keypair,
     const unsigned char *aux_rand32
 ) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4);
 
+/** Same as secp256k1_schnorrsig_sign32, but DEPRECATED. Will be removed in
+ *  future versions. */
+SECP256K1_API int secp256k1_schnorrsig_sign(
+    const secp256k1_context* ctx,
+    unsigned char *sig64,
+    const unsigned char *msg32,
+    const secp256k1_keypair *keypair,
+    const unsigned char *aux_rand32
+) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4)
+  SECP256K1_DEPRECATED("Use secp256k1_schnorrsig_sign32 instead");
+
 /** Create a Schnorr signature with a more flexible API.
  *
  *  Same arguments as secp256k1_schnorrsig_sign except that it allows signing

src/modules/schnorrsig/main_impl.h

@@ -192,11 +192,15 @@ static int secp256k1_schnorrsig_sign_internal(const secp256k1_context* ctx, unsi
     return ret;
 }
 
-int secp256k1_schnorrsig_sign(const secp256k1_context* ctx, unsigned char *sig64, const unsigned char *msg32, const secp256k1_keypair *keypair, const unsigned char *aux_rand32) {
+int secp256k1_schnorrsig_sign32(const secp256k1_context* ctx, unsigned char *sig64, const unsigned char *msg32, const secp256k1_keypair *keypair, const unsigned char *aux_rand32) {
     /* We cast away const from the passed aux_rand32 argument since we know the default nonce function does not modify it. */
     return secp256k1_schnorrsig_sign_internal(ctx, sig64, msg32, 32, keypair, secp256k1_nonce_function_bip340, (unsigned char*)aux_rand32);
 }
 
+int secp256k1_schnorrsig_sign(const secp256k1_context* ctx, unsigned char *sig64, const unsigned char *msg32, const secp256k1_keypair *keypair, const unsigned char *aux_rand32) {
+    return secp256k1_schnorrsig_sign32(ctx, sig64, msg32, keypair, aux_rand32);
+}
+
 int secp256k1_schnorrsig_sign_custom(const secp256k1_context* ctx, unsigned char *sig64, const unsigned char *msg, size_t msglen, const secp256k1_keypair *keypair, secp256k1_schnorrsig_extraparams *extraparams) {
     secp256k1_nonce_function_hardened noncefp = NULL;
     void *ndata = NULL;

src/modules/schnorrsig/tests_impl.h

@@ -160,21 +160,21 @@ void test_schnorrsig_api(void) {
 
     /** main test body **/
     ecount = 0;
-    CHECK(secp256k1_schnorrsig_sign(none, sig, msg, &keypairs[0], NULL) == 1);
+    CHECK(secp256k1_schnorrsig_sign32(none, sig, msg, &keypairs[0], NULL) == 1);
     CHECK(ecount == 0);
-    CHECK(secp256k1_schnorrsig_sign(vrfy, sig, msg, &keypairs[0], NULL) == 1);
+    CHECK(secp256k1_schnorrsig_sign32(vrfy, sig, msg, &keypairs[0], NULL) == 1);
     CHECK(ecount == 0);
-    CHECK(secp256k1_schnorrsig_sign(sign, sig, msg, &keypairs[0], NULL) == 1);
+    CHECK(secp256k1_schnorrsig_sign32(sign, sig, msg, &keypairs[0], NULL) == 1);
     CHECK(ecount == 0);
-    CHECK(secp256k1_schnorrsig_sign(sign, NULL, msg, &keypairs[0], NULL) == 0);
+    CHECK(secp256k1_schnorrsig_sign32(sign, NULL, msg, &keypairs[0], NULL) == 0);
     CHECK(ecount == 1);
-    CHECK(secp256k1_schnorrsig_sign(sign, sig, NULL, &keypairs[0], NULL) == 0);
+    CHECK(secp256k1_schnorrsig_sign32(sign, sig, NULL, &keypairs[0], NULL) == 0);
     CHECK(ecount == 2);
-    CHECK(secp256k1_schnorrsig_sign(sign, sig, msg, NULL, NULL) == 0);
+    CHECK(secp256k1_schnorrsig_sign32(sign, sig, msg, NULL, NULL) == 0);
     CHECK(ecount == 3);
-    CHECK(secp256k1_schnorrsig_sign(sign, sig, msg, &invalid_keypair, NULL) == 0);
+    CHECK(secp256k1_schnorrsig_sign32(sign, sig, msg, &invalid_keypair, NULL) == 0);
     CHECK(ecount == 4);
-    CHECK(secp256k1_schnorrsig_sign(sttc, sig, msg, &keypairs[0], NULL) == 0);
+    CHECK(secp256k1_schnorrsig_sign32(sttc, sig, msg, &keypairs[0], NULL) == 0);
     CHECK(ecount == 5);
 
     ecount = 0;
@@ -202,7 +202,7 @@ void test_schnorrsig_api(void) {
     CHECK(ecount == 6);
 
     ecount = 0;
-    CHECK(secp256k1_schnorrsig_sign(sign, sig, msg, &keypairs[0], NULL) == 1);
+    CHECK(secp256k1_schnorrsig_sign32(sign, sig, msg, &keypairs[0], NULL) == 1);
     CHECK(secp256k1_schnorrsig_verify(none, sig, msg, sizeof(msg), &pk[0]) == 1);
     CHECK(ecount == 0);
     CHECK(secp256k1_schnorrsig_verify(sign, sig, msg, sizeof(msg), &pk[0]) == 1);
@@ -247,7 +247,7 @@ void test_schnorrsig_bip_vectors_check_signing(const unsigned char *sk, const un
     secp256k1_xonly_pubkey pk, pk_expected;
 
     CHECK(secp256k1_keypair_create(ctx, &keypair, sk));
-    CHECK(secp256k1_schnorrsig_sign(ctx, sig, msg32, &keypair, aux_rand));
+    CHECK(secp256k1_schnorrsig_sign32(ctx, sig, msg32, &keypair, aux_rand));
     CHECK(secp256k1_memcmp_var(sig, expected_sig, 64) == 0);
 
     CHECK(secp256k1_xonly_pubkey_parse(ctx, &pk_expected, pk_serialized));
@@ -740,8 +740,11 @@ void test_schnorrsig_sign(void) {
     secp256k1_testrand256(aux_rand);
     CHECK(secp256k1_keypair_create(ctx, &keypair, sk));
     CHECK(secp256k1_keypair_xonly_pub(ctx, &pk, NULL, &keypair));
-    CHECK(secp256k1_schnorrsig_sign(ctx, sig, msg, &keypair, NULL) == 1);
+    CHECK(secp256k1_schnorrsig_sign32(ctx, sig, msg, &keypair, NULL) == 1);
     CHECK(secp256k1_schnorrsig_verify(ctx, sig, msg, sizeof(msg), &pk));
+    /* Check that deprecated alias gives the same result */
+    CHECK(secp256k1_schnorrsig_sign(ctx, sig2, msg, &keypair, NULL) == 1);
+    CHECK(secp256k1_memcmp_var(sig, sig2, sizeof(sig)) == 0);
 
     /* Test different nonce functions */
     CHECK(secp256k1_schnorrsig_sign_custom(ctx, sig, msg, sizeof(msg), &keypair, &extraparams) == 1);
@@ -764,7 +767,7 @@ void test_schnorrsig_sign(void) {
     extraparams.noncefp = NULL;
     extraparams.ndata = aux_rand;
     CHECK(secp256k1_schnorrsig_sign_custom(ctx, sig, msg, sizeof(msg), &keypair, &extraparams) == 1);
-    CHECK(secp256k1_schnorrsig_sign(ctx, sig2, msg, &keypair, extraparams.ndata) == 1);
+    CHECK(secp256k1_schnorrsig_sign32(ctx, sig2, msg, &keypair, extraparams.ndata) == 1);
     CHECK(secp256k1_memcmp_var(sig, sig2, sizeof(sig)) == 0);
 }
 
@@ -787,7 +790,7 @@ void test_schnorrsig_sign_verify(void) {
 
     for (i = 0; i < N_SIGS; i++) {
         secp256k1_testrand256(msg[i]);
-        CHECK(secp256k1_schnorrsig_sign(ctx, sig[i], msg[i], &keypair, NULL));
+        CHECK(secp256k1_schnorrsig_sign32(ctx, sig[i], msg[i], &keypair, NULL));
         CHECK(secp256k1_schnorrsig_verify(ctx, sig[i], msg[i], sizeof(msg[i]), &pk));
     }
 
@@ -816,13 +819,13 @@ void test_schnorrsig_sign_verify(void) {
     }
 
     /* Test overflowing s */
-    CHECK(secp256k1_schnorrsig_sign(ctx, sig[0], msg[0], &keypair, NULL));
+    CHECK(secp256k1_schnorrsig_sign32(ctx, sig[0], msg[0], &keypair, NULL));
     CHECK(secp256k1_schnorrsig_verify(ctx, sig[0], msg[0], sizeof(msg[0]), &pk));
     memset(&sig[0][32], 0xFF, 32);
     CHECK(!secp256k1_schnorrsig_verify(ctx, sig[0], msg[0], sizeof(msg[0]), &pk));
 
     /* Test negative s */
-    CHECK(secp256k1_schnorrsig_sign(ctx, sig[0], msg[0], &keypair, NULL));
+    CHECK(secp256k1_schnorrsig_sign32(ctx, sig[0], msg[0], &keypair, NULL));
     CHECK(secp256k1_schnorrsig_verify(ctx, sig[0], msg[0], sizeof(msg[0]), &pk));
     secp256k1_scalar_set_b32(&s, &sig[0][32], NULL);
     secp256k1_scalar_negate(&s, &s);
@@ -873,7 +876,7 @@ void test_schnorrsig_taproot(void) {
 
     /* Key spend */
     secp256k1_testrand256(msg);
-    CHECK(secp256k1_schnorrsig_sign(ctx, sig, msg, &keypair, NULL) == 1);
+    CHECK(secp256k1_schnorrsig_sign32(ctx, sig, msg, &keypair, NULL) == 1);
     /* Verify key spend */
     CHECK(secp256k1_xonly_pubkey_parse(ctx, &output_pk, output_pk_bytes) == 1);
     CHECK(secp256k1_schnorrsig_verify(ctx, sig, msg, sizeof(msg), &output_pk) == 1);

src/valgrind_ctime_test.c

@@ -166,7 +166,7 @@ void run_tests(secp256k1_context *ctx, unsigned char *key) {
     ret = secp256k1_keypair_create(ctx, &keypair, key);
     VALGRIND_MAKE_MEM_DEFINED(&ret, sizeof(ret));
     CHECK(ret == 1);
-    ret = secp256k1_schnorrsig_sign(ctx, sig, msg, &keypair, NULL);
+    ret = secp256k1_schnorrsig_sign32(ctx, sig, msg, &keypair, NULL);
     VALGRIND_MAKE_MEM_DEFINED(&ret, sizeof(ret));
     CHECK(ret == 1);
 #endif