schnorrsig API overhaul

jonasnick commented at 7:34 pm on November 2, 2020: contributor

This is a work in progress because I wanted to put this up for discussion before writing tests. It addresses the TODOs that didn’t make it in the schnorrsig PR and changes the APIs of schnorrsig_sign, schnorrsig_verify and hardened_nonce_function.

Ideally, the new aux_rand32 argument for sign would be const, but didn’t find a solution I was happy with.
Support for variable length message signing and verification supports the suggested BIP amendment for such messages.
sign_custom with its opaque config object allows adding more arguments later without having to change the API again. Perhaps there are other sensible customization options, but I’m thinking of sign-to-contract/covert-channel in particular. It would require adding the fields unsigned char *s2c_data32 and secp256k1_s2c_opening *s2c_opening to the config struct. The former is the data to commit to and the latter is written to by sign_custom. (EDIT: see below)

LLFourn cross-referenced this on Nov 12, 2020 from issue BIP340: clarify impact of pre-hashed messages, or support variable-length messages by sipa

LLFourn cross-referenced this on Nov 12, 2020 from issue (Tagged) Hashing and Signing by Oracle of Message by nkohen

jonasnick commented at 5:49 pm on December 14, 2020: contributor

ping :)

in include/secp256k1_schnorrsig.h:138 in 14565fbee0 outdated

158+ *        ndata: pointer to arbitrary data used by the nonce generation function
159+ *               (can be NULL). If it is non-NULL and
160+ *               secp256k1_nonce_function_bip340 is used, then ndata must be a
161+ *               pointer to 32-byte auxiliary randomness as per BIP-340.
162+ *       config: pointer to a config object.
163+ */

LLFourn commented at 9:22 pm on December 14, 2020:

Perhaps I am misunderstanding the doc comment syntax but something is not quite right here. It should say something like “instead of aux_rand32 it accepts a custom config…”. I think then the only “In” should be the config?

jonasnick commented at 6:20 pm on December 15, 2020:

You’re right, the doc for ndata and noncefp here should be removed in the last commit.

jonasnick commented at 10:58 pm on January 22, 2021:

fixed

sipa commented at 6:14 pm on December 18, 2020: contributor

Concept ACK

elichai commented at 9:10 am on December 19, 2020: contributor

Can we use an opaque “array” struct for the config instead of using malloc? this will still allow us to extend the struct while also supporting platforms without heap allocation

jonasnick commented at 9:00 pm on December 22, 2020: contributor

@elichai Yeah good point, I agree that we should avoid dynamic memory allocation… and was hoping for suggestions :)

The opaque array struct wouldn’t be fully backwards compatible because it only works if the caller is linked to the correct version of libsecp. In my limited understanding, I can count three reasonable options if we want to avoid malloc and be binary backwards compatible:

Use a versioned struct as in the Linux kernel. I implemented this variant in a branch but I don’t know enough of the implications, i.e. how well this works with ffi.
Make space for future additions in the object by making the opaque byte array extra large or adding dummy members if we’d use a public struct instead.
Allocate it on the scratch space and work on optionally providing stack space to the scratch space (instead of having it malloc). That’s not exactly backwards compatible either because the provided scratch space may become to small.

In case 1 and 2 we should document that the struct size may change between versions such that no one relies on this (otherwise it seems like this would become super complex)

in src/modules/schnorrsig/main_impl.h:212 in 14565fbee0 outdated

208+
209+void secp256k1_schnorrsig_config_destroy(const secp256k1_context* ctx, secp256k1_schnorrsig_config *config) {
210+    VERIFY_CHECK(ctx != NULL);
211+
212+    if (config != NULL) {
213+        free(config);

real-or-random commented at 1:57 pm on January 5, 2021:

fwiw, you don’t need to check for NULL http://port70.net/~nsz/c/c89/c89-draft.html#4.10.3.2

in include/secp256k1_schnorrsig.h:59 in 14565fbee0 outdated

59- *  to "BIP0340/nonce\0\0\0"
60+ *  schnorrsig_sign does not follow the BIP-340 nonce derivation procedure
61+ *  exactly. The algo16 argument must be non-NULL, otherwise the function will
62+ *  fail and return 0. The hash will be tagged with algo16 after removing all
63+ *  terminating null bytes. Therefore, to create BIP-340 compliant signatures,
64+ *  algo16 must be set to "BIP0340/nonce\0\0\0"

real-or-random commented at 2:02 pm on January 5, 2021:

Maybe it’s easier than having a length but I think if we have a length arg for the message, we can have it here too. \0\0\0 is somewhat strange. I mean it works but stripping trailing \0 bytes is very unconventional, I haven’t seen this anywhere else.

jonasnick commented at 10:59 pm on January 22, 2021:

added algolen argument

real-or-random commented at 2:10 pm on January 5, 2021: contributor

Am I right that the user can still create config objects simply on the stack? This should be the case, we shouldn’t force the user to have malloc.

But if the answer to this question is yes, then I think we could get rid of the create/destroy functions entirely. Config objects will be small, it’s fine and natural to use the stack. And if the user really wants dynamic memory, she can call still call malloc.

Or do we think the size of the config object isn’t statically given? This is true for example for the context object. Here, the size depends on the flags. (Although you could argue that there are only four types of contexts with known static sizes…) But here I don’t think this will be case for config objects.

real-or-random commented at 2:28 pm on January 5, 2021: contributor

Ah sorry, I haven’t seen your previous comment @jonasnick.

I think what I have in mind is similar to 1. but even simpler.

As long as we keep this an opaque object with manipulation functions provided by us, and we give the caller a way to figure out it’s size (e.g. for FFI/dynamic alloc), versioning should not be necessary. Am I right?

apoelstra commented at 5:04 pm on January 5, 2021: contributor

@real-or-random the problem is that the “manipulation functions provided by us” need to use malloc.

jonasnick commented at 10:51 pm on January 8, 2021: contributor

I implemented & pushed option 1, the versioned struct, after playing with it here. The only downside is that if we add fields, we have to make sure that the new struct is in fact larger than the old one - on all supported compilers and platforms. We already make an assumption on the biggest alignment, so that should help figuring out when to add dummy fields.

I also mentioned above that there would be issues with FFI, but there are actually none. The concern in the issue I linked to was essentially that a struct could change out from under the bindings, but that’s exactly what versioned structs prevent.

I’ll add the missing tests and documentation.

As long as we keep this an opaque object @real-or-random callers can’t put an opaque object on the stack because they can’t statically determine its size.

jonasnick force-pushed on Jan 8, 2021

jonasnick force-pushed on Jan 22, 2021

jonasnick commented at 10:58 pm on January 22, 2021: contributor

I pushed a new version of this PR with two major changes:

The versioned struct for extra sign_custom parameters is removed. Versioned structs are useful for forward compatibility (i.e. application expecting newer library actually calls older library), but it’s not clear what to do in case we add sign to contract members to the struct, but the library doesn’t understand S2C yet. We can abort, but that’s not really forward compatibility. Instead it’s a simple struct now with a magic value which can be used to achieve backward compatibility. If we decide to change the struct, we also change the magic which then acts as a version number and allows libsecp to distinguish between different structs.
The schnorrsig_sign function only accepts 32-byte messages again. Also, I added the public secp256k1_tagged_sha256 function to create tagged hashes according to BIP-340. The schnorrsig_sign doc explains that for messages that are not 32 bytes, one should use secp256k1_tagged_sha256 with a domain seperator. schnorrsig_sign_custom still allows truly signing variable length messages. As such, with libsecp both methods for variable length messages can be used, but one is clearly recommended to not confuse users and that’s the one that currently has the most adoption (Taproot, DLCs, Lightning offers). And the additional tag argument to secp256k1_tagged_sha256 makes it harder to miss domain separation.

I also added tests and docs. Since this PR doesn’t contradict the discussed BIP-340 changes, I removed the WIP status.

jonasnick renamed this:
~~WIP: schnorrsig API overhaul~~
schnorrsig API overhaul
on Jan 22, 2021

real-or-random cross-referenced this on Feb 1, 2021 from issue Add doc/release-process.md by jonasnick

jonasnick cross-referenced this on Feb 8, 2021 from issue Add ECDSA adaptor signatures module by jesseposner

in include/secp256k1_schnorrsig.h:28 in dd7157ad03 outdated

31- *           algo16:    pointer to a 16-byte array describing the signature
32- *                      algorithm (will not be NULL).
33- *           data:      Arbitrary data pointer that is passed through.
34+ *  Out:  nonce32: pointer to a 32-byte array to be filled by the function.
35+ *  In:       msg: the message being verified (will not be NULL)
36+ *         msglen: the length of the message (will not be NULL)

jesseposner commented at 0:55 am on February 18, 2021:

0 *  In:       msg: the message hash being verified (will not be NULL)
1 *         msglen: the length of the message hash (will not be NULL)

real-or-random commented at 1:01 am on February 18, 2021:

No, messages in BIP340 can be arbitrary bytestrings. They do not need to be obtained via hashing.

jesseposner commented at 11:21 pm on February 18, 2021:

Ah, right, hence this PR. Missed that somehow on my first pass.

real-or-random commented at 10:54 am on February 19, 2021:

Ah no, I think there’s still a misunderstanding. So with the scheme in the current BIP340, you can already sign arbitrary 32-byte messages (no matter if they’re created via a hash function or not).

There have been changes proposed to BIP340 (not yet in the BIP) that extend this to arbitrary-length messages. This is what is implemented by this PR.

But BIP340 never required the messages to be created via a hash function. This is just a difference to the ECDSA API.

jesseposner commented at 5:12 pm on February 19, 2021:

Thanks for clarifying.

So given that BIP340 does not require the message to be hashed, the existing documentation on line 27 that specifies a 32-byte message hash is not strictly true, in the sense that a 32-byte non-hashed message is permissible.

However, I’m assuming that was not a documentation error (or was it?), because, when 32-bytes are required, for typical use cases (i.e. the message is not exactly 32-bytes) the message should be hashed.

This seems to also be reflected in the insertion of hash in this PR on line 109, because that API still requires a 32-byte message (despite BIP340 not requiring a hashed message, and with the proposed changes, not requiring a 32-byte message).

On the other hand, perhaps it would be more clear to drop the insertion of hash on line 109.

jonasnick commented at 10:30 pm on February 22, 2021:

I’m not sure I follow, what exactly is the possible documentation error? I agree that “message hash” for schnorrsig_sign’s msg32 argument is misleading because it’s not necessary that msg32 is one. Switched it back.

jesseposner commented at 5:14 pm on February 23, 2021:

The possible documentation error I was referring to is that currently line 27 on master says: https://github.com/bitcoin-core/secp256k1/blob/3a8b47bc6d1627a8f9f008234c5161538ef85a21/include/secp256k1_schnorrsig.h#L27 when it should probably be the 32-byte message being verified.

With your latest changes the documentation for both secp256k1_nonce_function_hardened and secp256k1_schnorrsig_sign now have message instead of message hash so it now looks good to me.

in include/secp256k1_schnorrsig.h:26 in dd7157ad03 outdated

29- *      xonly_pk32:     the 32-byte serialized xonly pubkey corresponding to key32
30- *                      (will not be NULL)
31- *           algo16:    pointer to a 16-byte array describing the signature
32- *                      algorithm (will not be NULL).
33- *           data:      Arbitrary data pointer that is passed through.
34+ *  Out:  nonce32: pointer to a 32-byte array to be filled by the function.

jesseposner commented at 1:01 am on February 18, 2021:

0 *  Out:  nonce32: pointer to a 32-byte array to be filled by the function

jonasnick commented at 10:30 pm on February 22, 2021:

done

in include/secp256k1_schnorrsig.h:33 in dd7157ad03 outdated

36+ *         msglen: the length of the message (will not be NULL)
37+ *          key32: pointer to a 32-byte secret key (will not be NULL)
38+ *     xonly_pk32: the 32-byte serialized xonly pubkey corresponding to key32
39+ *                 (will not be NULL)
40+ *           algo: pointer to an array describing the signature
41+ *                 algorithm (will not be NULL).

jesseposner commented at 1:02 am on February 18, 2021:

0 *                 algorithm (will not be NULL)

jonasnick commented at 10:30 pm on February 22, 2021:

done

in include/secp256k1_schnorrsig.h:35 in dd7157ad03 outdated

38+ *     xonly_pk32: the 32-byte serialized xonly pubkey corresponding to key32
39+ *                 (will not be NULL)
40+ *           algo: pointer to an array describing the signature
41+ *                 algorithm (will not be NULL).
42+ *        algolen: the length of the algo array
43+ *           data: Arbitrary data pointer that is passed through.

jesseposner commented at 1:02 am on February 18, 2021:

0 *           data: arbitrary data pointer that is passed through

jonasnick commented at 10:30 pm on February 22, 2021:

done

in include/secp256k1_schnorrsig.h:105 in dd7157ad03 outdated

107- *  randomness.
108+ *  This function only signs 32-byte messages. If you have messages of a
109+ *  different size, it is recommended to create a 32-byte message hash with
110+ *  secp256k1_tagged_sha256 and then sign the hash. Tagged hashing allows
111+ *  providing an application-specific tag for domain separation. This prevents
112+ *  signatures from being valid in multiple applications by accident.

jesseposner commented at 7:17 pm on February 19, 2021:

This paragraph might make it sound like application-specific tags are only relevant for non-32-byte messages, however, tags should be considered regardless of whether the message is hashed.

Shouldn’t the documentation recommend 1 of 2 options? (1) a tagged hash of the message with an application-specific tag or (2) an unhashed message with an application-specific tag prefix.

Perhaps this would be more clear:

0 *  This function only signs 32-byte messages. If you have messages of a
1 *  different size (or the same size but without an application-specific tag prefix), it is recommended to create a 32-byte message hash with
2 *  secp256k1_tagged_sha256 and then sign the hash. Tagged hashing allows
3 *  providing an application-specific tag for domain separation. This prevents
4 *  signatures from being valid in multiple applications by accident.

jonasnick commented at 10:33 pm on February 22, 2021:

You’re right, added your suggestion. The subtle recommendation right now from the api is the tagged hash, because that’s the non _custom variant of schnorrsig sign.

jesseposner referenced this in commit 640418998f on Feb 24, 2021

jesseposner referenced this in commit 0f84518bff on Feb 25, 2021

jonasnick force-pushed on Feb 25, 2021

jonasnick commented at 8:31 pm on February 25, 2021: contributor

Rebased on top of master because cirrus wasn’t included in this branch.

apoelstra approved

apoelstra commented at 8:12 pm on March 12, 2021: contributor

ack 64503686a26e21dda422fe5377537601d0f86aa6

sipa commented at 4:15 am on March 17, 2021: contributor

API looks good, code review ACK. Feel like squashing in the fixup?

jonasnick force-pushed on Mar 17, 2021

jonasnick commented at 10:01 am on March 17, 2021: contributor

Done.

in include/secp256k1_schnorrsig.h:54 in 0cda011bb0 outdated

54- *  argument must be non-NULL, otherwise the function will fail and return 0.
55- *  The hash will be tagged with algo16 after removing all terminating null
56- *  bytes. Therefore, to create BIP-340 compliant signatures, algo16 must be set
57- *  to "BIP0340/nonce\0\0\0"
58+ *  schnorrsig_sign does not follow the BIP-340 nonce derivation procedure
59+ *  exactly. The algo16 argument must be non-NULL, otherwise the function will

real-or-random commented at 11:46 am on March 17, 2021:

This may sound scary. AFAIU the only change is that then we assume 32 zero bytes as aux rand. If that’s true, I suggest just stating this. I think then it’s a stretch to say that this does not follow BIP340. The BIP says you can set this to 32 zero bytes.

jonasnick commented at 2:52 pm on March 19, 2021:

Yeah, I improved the sentence.

in src/modules/schnorrsig/tests_impl.h:89 in 2bc57d7bad outdated

104-    CHECK(nonce_function_bip340(nonce, msg, key, pk, algo16, NULL) == 1);
105+    /* NULL algo is disallowed */
106+    CHECK(nonce_function_bip340(nonce, msg, key, pk, NULL, 0, NULL) == 0);
107+    /* Empty algo is fine */
108+    memset(algo, 0x00, algolen);
109+    CHECK(nonce_function_bip340(nonce, msg, key, pk, algo, algolen, NULL) == 1);

real-or-random commented at 12:31 pm on March 17, 2021:

Hm now that we don’t use \0-terminated strings, that’s not really a test for “empty” now.

jonasnick commented at 2:55 pm on March 19, 2021:

Removed that. algolen == 0 is tested a few lines below.

in include/secp256k1_schnorrsig.h:90 in c6f85508b7 outdated

94+) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4);
95+
96+/** Create a Schnorr signature with a more flexible API.
97+ *
98+ *  Same arguments as secp256k1_schnorrsig_sign except that it misses aux_rand32
99+ *  and instead allows allows providing a different nonce derivation function

real-or-random commented at 12:37 pm on March 17, 2021:

s/allows allows/allows s/misses/lacks (Not entirely sure because I’m not native speaker but “lacks” should be good in any case.)

jonasnick commented at 2:56 pm on March 19, 2021:

This is fixed in a later commit. Can I leave as is?

in src/bench_schnorrsig.c:62 in 46f3d69a98 outdated

60@@ -60,7 +61,7 @@ int main(void) {
61

real-or-random commented at 1:02 pm on March 17, 2021:

0    CHECK(MSGLEN >= 2);

jonasnick commented at 2:56 pm on March 19, 2021:

There was no intention to allow users to change MSGLEN and this wouldn’t work since the benchmark only used schnorrsig_sign, not schnorrsig_sign_custom. It may be useful though, so I added a commit to allow that.

in src/bench_schnorrsig.c:30 in 46f3d69a98 outdated

26@@ -26,7 +27,7 @@ typedef struct {
27 void bench_schnorrsig_sign(void* arg, int iters) {
28     bench_schnorrsig_data *data = (bench_schnorrsig_data *)arg;
29     int i;
30-    unsigned char msg[32] = "benchmarkexamplemessagetemplate";
31+    unsigned char msg[MSGLEN] = "benchmarkexamplemessagetemplate";

real-or-random commented at 1:14 pm on March 17, 2021:

maybe

0    unsigned char msg[MSGLEN] = {0};

because people could set MSGLEN to < 32

jonasnick commented at 2:56 pm on March 19, 2021:

done

in include/secp256k1_schnorrsig.h:27 in 46f3d69a98 outdated

23@@ -24,7 +24,8 @@ extern "C" {
24  *  Returns: 1 if a nonce was successfully generated. 0 will cause signing to
25  *           return an error.
26  *  Out:  nonce32: pointer to a 32-byte array to be filled by the function
27- *  In:     msg32: the 32-byte message hash being verified (will not be NULL)
28+ *  In:       msg: the message being verified (will not be NULL)

real-or-random commented at 1:16 pm on March 17, 2021:

will not be NULL unless msglen == 0

jonasnick commented at 2:57 pm on March 19, 2021:

good catch, fixed

in src/modules/schnorrsig/tests_impl.h:759 in b073fb7542 outdated

754+     * the same result as schnorrsig_sign with aux_rand = extraparams.ndata */
755+    extraparams.noncefp = NULL;
756+    extraparams.ndata = aux_rand;
757+    CHECK(secp256k1_schnorrsig_sign_custom(ctx, sig, msg, sizeof(msg), &keypair, &extraparams) == 1);
758+    CHECK(secp256k1_schnorrsig_sign(ctx, sig2, msg, &keypair, extraparams.ndata) == 1);
759+    CHECK(memcmp(sig, sig2, sizeof(sig)) == 0);

real-or-random commented at 1:24 pm on March 17, 2021:

secp256k1_memcmp_var

jonasnick commented at 2:57 pm on March 19, 2021:

ok

in include/secp256k1_schnorrsig.h:105 in b073fb7542 outdated

108+ *  This function only signs 32-byte messages. If you have messages of a
109+ *  different size (or the same size but without an application-specific tag
110+ *  prefix), it is recommended to create a 32-byte message hash with
111+ *  secp256k1_tagged_sha256 and then sign the hash. Tagged hashing allows
112+ *  providing an application-specific tag for domain separation. This prevents
113+ *  signatures from being valid in multiple applications by accident.

real-or-random commented at 1:52 pm on March 17, 2021:

okay maybe that’s nitpicking but can you change this “in multiple applications or in multiple contexts within the same application by accident”. I think people should understand that this can be meaningful within an applications (modulo the anyway imprecise meaning of this term).

jonasnick commented at 2:58 pm on March 19, 2021:

Agree, but not really sure about your suggestion because we call it an “application-specific” tag. Instead, I replaced “application” with “context” which actually matches the wording in bip-340. What do you think?

real-or-random commented at 1:28 pm on March 23, 2021:

Indeed, that’s better. I didn’t want to suggest “context” because I thought it could be confused with the meaning of context as in “secp256k1 context” but that shouldn’t be an issue.

in src/tests.c:570 in b073fb7542 outdated

565+
566+    /* Static test vector */
567+    memcpy(tag, "tag", 3);
568+    memcpy(msg, "msg", 3);
569+    CHECK(secp256k1_tagged_sha256(none, hash32, tag, 3, msg, 3) == 1);
570+    CHECK(memcmp(hash32, hash_expected, sizeof(hash32)) == 0);

real-or-random commented at 1:55 pm on March 17, 2021:

same here. (I should really work on #833 :P)

jonasnick commented at 2:58 pm on March 19, 2021:

fixed

real-or-random commented at 1:56 pm on March 17, 2021: contributor

I like this approach.

Can you a explain how the extraparams struct is now forward-compatible or not? (Here on even as comments in the source?)

jonasnick commented at 2:59 pm on March 19, 2021: contributor

There’s no forward compatibility, i.e. applications expecting a newer library than installed won’t work. Adding a new field to the extraparams requires a bump of the minor version. Then, applications which need that feature can demand at least that version.

jonasnick force-pushed on Mar 19, 2021

jonasnick commented at 7:41 pm on March 19, 2021: contributor

Rebased to (hopefully) fix CI which was complaining about invalid bignum selection.

elichai commented at 12:30 pm on March 20, 2021: contributor

I’m not sure I understand the problems this new API is solving

What’s the point of exposing secp256k1_tagged_sha256? we currently don’t expose regular sha256 so why should we expose a tagged one?.
What does the new secp256k1_schnorrsig_extraparams struct help solve? why is it better over the old API? (noncefp+ndata).

Thanks! (Other than that the implementation looks good)

EDIT I guess with regards to 2, it allows us to add new optional fields to the struct in a backwards compatible manner.

jonasnick commented at 2:19 pm on March 20, 2021: contributor

What’s the point of exposing secp256k1_tagged_sha256?

Because the recommended signing procedure is to hash the message with a context-specific tag and then sign that. If we didn’t expose secp256k1_tagged_sha256, that would be unnecessarily difficult for users.

What does the new secp256k1_schnorrsig_extraparams struct help solve? I guess […] it allows us to add new optional fields

Right, for example, we may want to use the extraparams to allow adding a sign-to-contract commitment to the signature or change the exact signing algorithm in other ways. The same could be achieved without extraparams by just adding entirely new signing functions sign_1, ..., sign_n with ever growing argument lists.

elichai commented at 2:40 pm on March 20, 2021: contributor

Because the recommended signing procedure is to hash the message with a context-specific tag and then sign that. If we didn’t expose secp256k1_tagged_sha256, that would be unnecessarily difficult for users.

So the use case is something like the following?

0typedef struct {const secp256k1_context* ctx; unsigned char[32] hash;} custom_data;
1// This function is used as `secp256k1_nonce_function_hardened`
2int my_custom_nonce_func(unsigned char *nonce32, const unsigned char *msg, size_t msglen, const unsigned char *key32, const unsigned char *xonly_pk32, const unsigned char *algo, size_t algolen, void *data) {
3    // Do custom own processing and result in a full message in the pair `(should_sign_ptr, should_sign_len)`.
4    // ...
5    custom_data* my_custom_data = (custom_data*)data;
6    return secp256k1_tagged_sha256(my_custom_data->ctx, my_custom_data->hash, MY_OWN_APP_TAG, sizeof(MY_OWN_APP_TAG), should_sign_ptr, should_sign_len);
7}

jonasnick commented at 5:45 pm on March 20, 2021: contributor

The intended use case is simply signing non-32-byte messages:

0unsigned char msg[33] = {0};
1unsigned char msghash[32];
2secp256k1_tagged_sha256(ctx, msghash, MY_OWN_APP_TAG, sizeof(MY_OWN_APP_TAG), msg, sizeof(msg));
3secp256k1_schnorrsig_sign(ctx, ..., msghash, ...);

You could also use sign_custom with msglen = 33 but as mentioned above “one is clearly recommended to not confuse users and that’s the one that currently has the most adoption (Taproot, DLCs, Lightning offers). And the additional tag argument to secp256k1_tagged_sha256 makes it harder to miss domain separation.”

in include/secp256k1_schnorrsig.h:59 in 4891704057 outdated

58- *  argument must be non-NULL, otherwise the function will fail and return 0.
59- *  The hash will be tagged with algo16 after removing all terminating null
60- *  bytes. Therefore, to create BIP-340 compliant signatures, algo16 must be set
61- *  to "BIP0340/nonce\0\0\0"
62+ *  the nonce derivation procedure follows BIP-340 by setting the auxiliary
63+ *  random data to zero. The algo argument must be non-NULL, otherwise the

real-or-random commented at 1:32 pm on March 23, 2021:

I was about to suggest that maybe we could allow algo == NULL if algolen == 0 But I’m not sure. If don’t think we want to encourage users to use empty strings…

real-or-random commented at 1:34 pm on March 23, 2021: contributor

Fixups are all good, fine to squash from my side.

jonasnick force-pushed on Mar 23, 2021

jonasnick commented at 2:14 pm on March 23, 2021: contributor

squashed

apoelstra commented at 4:09 pm on May 25, 2021: contributor

Needs rebase

README: mention schnorrsig module 99e8614812

schnorrsig: clarify result of calling nonce_function_bip340 without data df3bfa12c3

schnorrsig: add algolen argument to nonce_function_hardened

This avoids having to remove trailing NUL bytes in the nonce function

442cee5baf

jonasnick force-pushed on May 28, 2021

jonasnick commented at 11:41 am on May 28, 2021: contributor

rebased

jonasnick force-pushed on May 28, 2021

jonasnick commented at 5:37 pm on May 28, 2021: contributor

The new leak sanitizer in CI detected a missing context_destroy in the tagged_sha256_tests. Fixed & squashed.

apoelstra approved

apoelstra commented at 3:25 pm on June 10, 2021: contributor

ack 439ed2ff2fa3fe38bb0aeb26213d7f2263eca701

Glad we came to a “resolution” on the forward-compat issue.

in include/secp256k1_schnorrsig.h:30 in 442cee5baf outdated

33- *           data:      Arbitrary data pointer that is passed through.
34+ *  Out:  nonce32: pointer to a 32-byte array to be filled by the function
35+ *  In:     msg32: the 32-byte message hash being verified (will not be NULL)
36+ *          key32: pointer to a 32-byte secret key (will not be NULL)
37+ *     xonly_pk32: the 32-byte serialized xonly pubkey corresponding to key32
38+ *                 (will not be NULL)

ariard commented at 1:36 pm on June 23, 2021:

What’s the arguments handling difference between “will not be NULL” and “cannot be NULL” as annotated on top of secp256k1_schnorrsig_sign ?

Like should every will-not-be-null argument be sanitized with ARG_CHECK ?

jonasnick commented at 9:37 pm on June 23, 2021:

This is the type a function that is user provided and called from inside the library. Hence, the documentation says that this function will not be called with certain arguments set to NULL.

in include/secp256k1_schnorrsig.h:114 in 1bf261963a outdated

79- *         ndata: pointer to arbitrary data used by the nonce generation
80- *                function (can be NULL). If it is non-NULL and
81- *                secp256k1_nonce_function_bip340 is used, then ndata must be a
82- *                pointer to 32-byte auxiliary randomness as per BIP-340.
83+ *    aux_rand32: 32 bytes of fresh randomness. While recommended to provide
84+ *                this, it is only supplemental to security and can be NULL. See

ariard commented at 2:01 pm on June 23, 2021:

nits: “it is only a side-channel mitigation”, “See BIP-340 “Default Signing”

jonasnick commented at 9:39 pm on June 23, 2021:

Added the latter

in include/secp256k1.h:809 in b12a004897 outdated

804+ *  Returns 0 if the arguments are invalid and 1 otherwise.
805+ *  Args:    ctx: pointer to a context object
806+ *  Out:  hash32: pointer to a 32-byte array to store the resulting hash
807+ *  In:      tag: pointer to an array containing the tag
808+ *        taglen: length of the tag array
809+ *           msg: pointer to an array containing the message

ariard commented at 2:15 pm on June 23, 2021:

tag and msg maybe annotate “cannot be NULL” ?

jonasnick commented at 9:37 pm on June 23, 2021:

This was written with #783 already in mind which makes “cannot be NULL” implicit.

in include/secp256k1_schnorrsig.h:27 in 4fd25f00eb outdated

23@@ -24,7 +24,8 @@ extern "C" {
24  *  Returns: 1 if a nonce was successfully generated. 0 will cause signing to
25  *           return an error.
26  *  Out:  nonce32: pointer to a 32-byte array to be filled by the function
27- *  In:     msg32: the 32-byte message hash being verified (will not be NULL)
28+ *  In:       msg: the message being verified. Can be NULL iff msglen is 0.

ariard commented at 2:16 pm on June 23, 2021:

nit: s/iff/if/g

jonasnick commented at 9:39 pm on June 23, 2021:

expanded to if and only if

in include/secp256k1_schnorrsig.h:72 in 4fd25f00eb outdated

67@@ -66,6 +68,13 @@ SECP256K1_API extern const secp256k1_nonce_function_hardened secp256k1_nonce_fun
68  *  signature. Instead, you can manually use secp256k1_schnorrsig_verify and
69  *  abort if it fails.
70  *
71+ *  This function only signs 32-byte messages. If you have messages of a
72+ *  different size (or the same size but without an context-specific tag

ariard commented at 2:17 pm on June 23, 2021:

nit: s/an/a/g

jonasnick commented at 9:39 pm on June 23, 2021:

fixed

ariard commented at 2:49 pm on June 23, 2021: none

Mostly nits, SGTM

ariard commented at 10:46 pm on June 24, 2021: none

utACK 66ff2fa6

Thanks for taking the nits.

apoelstra commented at 10:52 pm on June 25, 2021: contributor

utACK the fixup commit

jonasnick force-pushed on Jun 27, 2021

jonasnick commented at 8:25 pm on June 27, 2021: contributor

squashed the comment fixup commit

schnorrsig: remove noncefp args from sign; add sign_custom function

This makes the default sign function easier to use while allowing more granular
control through sign_custom.

Tests for sign_custom follow in a later commit.

b6c0b72fb0

Add secp256k1_tagged_sha256 as defined in BIP-340

Gives users the ability to hash messages to 32 byte before they are signed while
allowing efficient domain separation through the tag.

5a8e4991ad

schnorrsig: allow signing and verification of variable length msgs

Varlen message support for the default sign function comes from recommending
tagged_sha256. sign_custom on the other hand gets the ability to directly sign
message of any length. This also implies signing and verification support for
the empty message (NULL) with msglen 0.

Tests for variable lengths follow in a later commit.

a0c3fc177f

schnorrsig: add extra parameter struct for sign_custom

This simplifies the interface of sign_custom and allows adding more parameters
later in a backward compatible way.

d8d806aaf3

schnorrsig: add tests for sign_custom and varlen msg verification fdd06b7967

schnorrsig: allow setting MSGLEN != 32 in benchmark 5f6ceafcfa

real-or-random requested review from real-or-random on Jun 28, 2021

apoelstra approved

apoelstra commented at 5:02 pm on June 29, 2021: contributor

ack 5f6ceafcfa46a69e901bed87e2c5f323b03b1e8c

ariard commented at 12:35 pm on June 30, 2021: none

utACK 5f6ceaf

LLFourn approved

LLFourn commented at 0:01 am on July 1, 2021: none

utACK 5f6ceafcfa46a69e901bed87e2c5f323b03b1e8c

real-or-random merged this on Jul 3, 2021

real-or-random closed this on Jul 3, 2021

real-or-random cross-referenced this on Jul 3, 2021 from issue SECP256K1_SCHNORRSIG_EXTRAPARAMS_INIT does not work with C++ by guidovranken

real-or-random cross-referenced this on Jul 4, 2021 from issue "Schnorrsig API overhaul" fixups by real-or-random

sipa cross-referenced this on Jul 14, 2021 from issue Update libsecp256k1 subtree to latest upstream by sipa

apoelstra cross-referenced this on Jul 27, 2021 from issue Upstream PRs 879, 959, 955, 944, 951, 960, 844, 963, 965 by apoelstra

fanquake referenced this in commit 71797beec5 on Aug 3, 2021

sidhujag referenced this in commit ce5d5632a3 on Aug 4, 2021

UdjinM6 referenced this in commit 54f3b66902 on Aug 10, 2021

5tefan referenced this in commit 235da293a2 on Aug 12, 2021

schnorrsig API overhaul #844