Add an experimental schnorr signature adaptor module #268
Conversation
|
Concept ACK. It might be interesting to include a |
this would be great :) |
|
@ssantos21 @instagibbs Thanks for the advice! I'll for sure add the example code for Schnorr Adaptor. |
|
This pull request is still under review and most likely will have some modifications. Feel free to give me more suggestions and advice! |
|
The current const time test is failing because:
I have fixed these issues in this commit: siv2r@ad1cc6e. Feel free to cherry-pick it using git or simply copy the changes :) The commit also adds const-time tests for the |
|
Pushed an example file, in the pattern of proposed LN PTLC usage: https://github.com/instagibbs/secp256k1-zkp/tree/schnorr-adaptor-signature-example feel free to cherry-pick as well |
add schnorr adaptor nonce function that follows BIP-340 and also accepts the adaptor point as input add presign, extract_t, adapt, and extract_adaptor APIs add tests for schnorr adaptor nonce functions and APIs
…daptor point in the nonce function
added new hash tags specifically for schnorr adaptor
ZhePang
force-pushed
the
schnorr-adaptor-signature
branch
from
d268b56
to
ed3e2ae
Compare
There was a problem hiding this comment.
Over all, what I found challenging using this new feature is the naming of function arguments and the corresponding comments. It led to confusion whether a function takes a pre signature or the Schnorr signature (sig64 vs sig65). Same goes for adaptor point (t33) vs. adaptor (t32)
sig65: Should always named the same. Either "serialized adaptor signature" or "adaptor signature", never just "serialized signature".
|
I was playing around with this, and it's great! Though one issue I encountered is that the |
Hi @Tibo-lg , we expect users to call |
Actually we don't. Sorry, I was wrong when we talked about this. Typically, the party that created the adaptor signature would call |
There was a problem hiding this comment.
I've just completed a review of your code and noticed a few areas that could benefit from refinement. I did not find any serious issues. It would be good if we could add another test vector as mentioned in the review comments. As promised, I'll also follow-up with an attempt at a module level documentation.
To further enhance your code's readability, you may want to tweak some of the code comments in the presign, extract, adapt, and extract_sec functions.
|
Here's my suggestion for a module level documentation (to be put at the top of include/secp256k1_schnorr_adaptor.h): /** This module provides an experimental implementation of a Schnorr adaptor
* signature protocol variant.
*
* The test vectors have been generated and cross-verified using a Python
* implementation of this adaptor signature variant available at [0].
*
* The protocol involves two parties, Alice and Bob. The general sequence of
* their interaction is as follows:
* 1. Alice calls the `schnorr_adaptor_presign` function for an adaptor point T
* and sends the pre-signature to Bob.
* 2. Bob extracts the adaptor point T from the pre-signature using
* `schnorr_adaptor_extract`.
* 3. Bob provides the pre-signature and the discrete logarithm of T to
* `schnorr_adaptor_adapt` which outputs a valid BIP 340 Schnorr signature.
* 4. Alice extracts the discrete logarithm of T from the pre-signature and the
* BIP 340 signature using `schnorr_adaptor_extract_sec`.
*
* In contrast to common descriptions of adaptor signature protocols, this
* module does not provide a verification algorithm for pre-signatures.
* Instead, `schnorr_adaptor_extract` returns the adaptor point encoded by a
* pre-signature, reducing communication cost. If a verification function for
* pre-signatures is needed, it can be easily simulated with
* `schnorr_adaptor_extract`.
*
* Assuming that BIP 340 Schnorr signatures satisfy strong unforgeability under
* chosen message attack, the Schnorr adaptor signature scheme fulfills the
* following properties as formalized by [1].
*
* - Witness extractability:
* If Alice
* 1. creates a pre-signature with `schnorr_adaptor_presign` for message m
* and adaptor point T and
* 2. receives a Schnorr signature for message m that she hasn't created
* herself,
* then Alice is able to obtain the discrete logarithm of T with
* `schnorr_adaptor_extract_sec`.
*
* - Pre-signature adaptability:
* If Bob
* 1. receives a pre-signature and extracts an adaptor point T using
* `schnorr_adaptor_extract`, and
* 2. obtains the discrete logarithm of the adaptor point T
* Then then Bob is able to adapt the received pre-signature to a valid BIP
* 340 Schnorr signature using `schnorr_adaptor_adapt`.
*
* - Existential Unforgeability:
* Bob is not able to create a BIP 340 signature from a pre-signature for
* adaptor T without knowing the discrete logarithm of T.
*
* - Pre-signature existiential unforgeability:
* Only Alice can create a pre-signature for her public key.
*
* [0] https://github.com/ZhePang/Python_Specification_for_Schnorr_Adaptor
* [1] https://eprint.iacr.org/2020/476.pdf
*/ |
| */ | ||
| SECP256K1_API const secp256k1_adaptor_nonce_function_hardened secp256k1_adaptor_nonce_function_bip340; | ||
|
|
||
| /** Create a Schnorr adaptor signature. |
There was a problem hiding this comment.
Would it make sense to replace the term "adaptor signature" with "pre-signature" to be consistent with the literature (see #268 (comment))?
|
|
||
| const secp256k1_adaptor_nonce_function_hardened secp256k1_adaptor_nonce_function_bip340 = adaptor_nonce_function_bip340; | ||
|
|
||
| static int secp256k1_schnorr_adaptor_presign_internal(const secp256k1_context *ctx, unsigned char *presig65, const unsigned char *msg32, const secp256k1_keypair *keypair, secp256k1_adaptor_nonce_function_hardened noncefp, const unsigned char *adaptor, void *ndata) { |
There was a problem hiding this comment.
| static int secp256k1_schnorr_adaptor_presign_internal(const secp256k1_context *ctx, unsigned char *presig65, const unsigned char *msg32, const secp256k1_keypair *keypair, secp256k1_adaptor_nonce_function_hardened noncefp, const unsigned char *adaptor, void *ndata) { | |
| static int secp256k1_schnorr_adaptor_presign_internal(const secp256k1_context *ctx, unsigned char *presig65, const unsigned char *msg32, const secp256k1_keypair *keypair, secp256k1_adaptor_nonce_function_hardened noncefp, const secp256k1_pubkey *adaptor, void *ndata) { |
The current design allows the user to input an invalid adaptor point. By making this arg as secp256k1_pubkey, we can catch an invalid 33-byte adaptor point during secp256k1_ec_pubkey_parse.
secp256k1_ecdsa_adaptor_encrypt API does the same.
There was a problem hiding this comment.
I am unsure if the change is necessary since we parse the adaptor point using secp256k1_ec_pubkey_parse inside the presign function anyway.
There was a problem hiding this comment.
The essence of this modification is to make a design choice about serialization/deserialization.
- Option 1: user handles the ser/de-ser of the adaptor point
- we receive the adaptor point as
secp256k1_pubkey(in presign & extract_adaptor)
- we receive the adaptor point as
- Option 2: adaptor interface handles the ser/de-ser of the adaptor point
- we receive the adaptor point as
unsigned_char *(in presign & extract_adaptor)
- we receive the adaptor point as
Leaving the ser/de-ser to the user makes more sense to me since we already do this for the pubkey point. What’s the libsecp norm here?
There was a problem hiding this comment.
I just realized there is already a comment regarding this: #268 (comment). We are going with Option 1.
There was a problem hiding this comment.
Yes, I'd say the libsecp norm is to allow avoiding unnecessary costly recomputations such as pubkey deserialization.
| /* NULL algo is disallowed */ | ||
| CHECK(adaptor_nonce_function_bip340(nonce, msg, key, t, pk, NULL, 0, NULL) == 0); | ||
| CHECK(adaptor_nonce_function_bip340(nonce, msg, key, t, pk, algo, algolen, NULL) == 1); | ||
| /* Other algo is fine */ |
There was a problem hiding this comment.
Some additional edge-case tests that could be added here.
/* Empty algo is fine */
memset(algo, 0x00, algolen);
CHECK(adaptor_nonce_function_bip340(nonce, msg, key, t, pk, algo, algolen, NULL) == 1);
/* Max algo is fine */
memset(algo, 0xFF, algolen);
CHECK(adaptor_nonce_function_bip340(nonce, msg, key, t, pk, algo, algolen, NULL) == 1);There was a problem hiding this comment.
I don't think this test significantly increases test coverage because the nonce function does relatively little with algo. If we test algo = 0 an algo 0xff...f, why not test the same for the other nonce function arguments?
There was a problem hiding this comment.
why not test the same for the other nonce function arguments?
I didn't consider this case. I suggested those tests because the ECDSA adaptor had them.
I take back my suggestion. The adaptor_nonce_function_bip340 function passes its arguments to the hash function without checking their values (even their validity), except for algo and aux. Thus, a min-max test for each args seems unnecessary.
There was a problem hiding this comment.
Should we add a test for a non-null aux_rand argument? The current tests don't seem to hit the if (data != NULL) branch in adaptor_nonce_function_bip340.
| CHECK_ILLEGAL(CTX, secp256k1_schnorr_adaptor_extract(CTX, t2, sig, msg, NULL)); | ||
| CHECK_ILLEGAL(CTX, secp256k1_schnorr_adaptor_extract(CTX, t2, sig, msg, &zero_pk)); | ||
|
|
||
| CHECK(secp256k1_schnorr_adaptor_adapt(CTX, sig64, sig, secadaptor) == 1); |
There was a problem hiding this comment.
nit: we can check this sig64 result using schnorrsig_verify after this line.
There was a problem hiding this comment.
It would be nice to have a use-case test. For example,
- Schnorrsig has
test_schnorrsig_taproot - ECDSA adaptor has
multi_hop_lock_tests - Musig2 adaptor has
scriptless_atomic_swap
There was a problem hiding this comment.
Yes, similar to the "correctness" suggestion here: #268 (comment)
| * "BIP0340/nonce" and algolen to 13. | ||
| */ | ||
| SECP256K1_API const secp256k1_adaptor_nonce_function_hardened secp256k1_adaptor_nonce_function_bip340; | ||
| /** A Schnorr Adaptor nonce generation function. */ |
There was a problem hiding this comment.
nit: we could just remove the reference to BIP340, instead of the whole paragraph. Something like:
A Schnorr Adaptor nonce generation function.
If a data pointer is passed, it is assumed to be a pointer to 32 bytes of auxiliary random data. If the data pointer is NULL, the nonce derivation procedure sets the auxiliary random data to zero. The algo argument must be non-NULL, otherwise the function will fail and return 0. The hash will be tagged with algo. Therefore, algo must be set to "SchnorrAdaptor/nonce" and algolen to 20.
| @@ -47,7 +47,7 @@ static void secp256k1_adaptor_nonce_function_bip340_sha256_tagged_aux(secp256k1_ | |||
| * by using the correct tagged hash function. */ | |||
| static const unsigned char adaptor_bip340_algo[20] = "SchnorrAdaptor/nonce"; | |||
There was a problem hiding this comment.
nit: adaptor_bip340_algo -> adaptor_algo
| @@ -57,7 +57,7 @@ SECP256K1_API const secp256k1_adaptor_nonce_function_hardened secp256k1_nonce_fu | |||
| * Out: presig65: pointer to a 65-byte array to store the adaptor signature. | |||
| * In: msg32: the 32-byte message being signed. | |||
| * keypair: pointer to an initialized keypair. | |||
| * adaptor33: pointer to a 33-byte compressed adaptor point. | |||
| * adaptor: pointer to an adaptor point. | |||
There was a problem hiding this comment.
nit: 344620e renamed adaptor to adaptor33. Just wanna make sure that this revert is intentional.
There was a problem hiding this comment.
There were a few warnings during compilation, but all tests passed successfully.
warnings
src/ctime_tests.c: In function ‘run_tests’:
src/ctime_tests.c:212:74: warning: passing argument 5 of ‘secp256k1_schnorr_adaptor_presign’ from incompatible pointer type [-Wincompatible-pointer-types]
212 | ret = secp256k1_schnorr_adaptor_presign(ctx, sig, msg, &keypair, t, NULL);
| ^
| |
| unsigned char *
In file included from src/ctime_tests.c:35:
src/../include/secp256k1_schnorr_adaptor.h:72:29: note: expected ‘const secp256k1_pubkey *’ {aka ‘const struct <anonymous> *’} but argument is of type ‘unsigned char *’
72 | const secp256k1_pubkey *adaptor,
| ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
In file included from src/secp256k1.c:896,
from src/bench_ecmult.c:8:
src/modules/schnorr_adaptor/main_impl.h: In function ‘secp256k1_schnorr_adaptor_extract’:
src/modules/schnorr_adaptor/main_impl.h:197:12: warning: unused variable ‘size’ [-Wunused-variable]
197 | size_t size = 33;
| ^~~~
In file included from src/secp256k1.c:896,
from src/tests.c:20:
src/modules/schnorr_adaptor/main_impl.h: In function ‘secp256k1_schnorr_adaptor_extract’:
src/modules/schnorr_adaptor/main_impl.h:197:12: warning: unused variable ‘size’ [-Wunused-variable]
197 | size_t size = 33;
| ^~~~
In file included from src/secp256k1.c:896,
from src/bench_internal.c:8:
src/modules/schnorr_adaptor/main_impl.h: In function ‘secp256k1_schnorr_adaptor_extract’:
src/modules/schnorr_adaptor/main_impl.h:197:12: warning: unused variable ‘size’ [-Wunused-variable]
197 | size_t size = 33;
| ^~~~
In file included from src/secp256k1.c:896,
from src/tests_exhaustive.c:23:
src/modules/schnorr_adaptor/main_impl.h: In function ‘secp256k1_schnorr_adaptor_extract’:
src/modules/schnorr_adaptor/main_impl.h:197:12: warning: unused variable ‘size’ [-Wunused-variable]
197 | size_t size = 33;
| ^~~~
In file included from src/secp256k1.c:896,
from src/tests.c:20:
src/modules/schnorr_adaptor/main_impl.h: In function ‘secp256k1_schnorr_adaptor_extract’:
src/modules/schnorr_adaptor/main_impl.h:197:12: warning: unused variable ‘size’ [-Wunused-variable]
197 | size_t size = 33;
| ^~~~
In file included from src/secp256k1.c:896:
src/modules/schnorr_adaptor/main_impl.h: In function ‘secp256k1_schnorr_adaptor_extract’:
src/modules/schnorr_adaptor/main_impl.h:197:12: warning: unused variable ‘size’ [-Wunused-variable]
197 | size_t size = 33;
| ^~~~
In file included from src/tests.c:7474:
src/modules/schnorr_adaptor/tests_impl.h: In function ‘test_schnorr_adaptor_api’:
src/modules/schnorr_adaptor/tests_impl.h:141:5: warning: ignoring return value of ‘secp256k1_ec_pubkey_parse’, declared with attribute warn_unused_result [-Wunused-result]
141 | secp256k1_ec_pubkey_parse(CTX, &t, adaptor33, 33);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from src/tests.c:7474:
src/modules/schnorr_adaptor/tests_impl.h: In function ‘test_schnorr_adaptor_vectors_check_presigning’:
src/modules/schnorr_adaptor/tests_impl.h:180:5: warning: ignoring return value of ‘secp256k1_ec_pubkey_parse’, declared with attribute warn_unused_result [-Wunused-result]
180 | secp256k1_ec_pubkey_parse(CTX, &adaptor, adaptor33, 33);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from src/tests.c:7474:
src/modules/schnorr_adaptor/tests_impl.h: In function ‘test_schnorr_adaptor_api’:
src/modules/schnorr_adaptor/tests_impl.h:141:5: warning: ignoring return value of ‘secp256k1_ec_pubkey_parse’, declared with attribute warn_unused_result [-Wunused-result]
141 | secp256k1_ec_pubkey_parse(CTX, &t, adaptor33, 33);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from src/tests.c:7474:
src/modules/schnorr_adaptor/tests_impl.h: In function ‘test_schnorr_adaptor_vectors_check_presigning’:
src/modules/schnorr_adaptor/tests_impl.h:180:5: warning: ignoring return value of ‘secp256k1_ec_pubkey_parse’, declared with attribute warn_unused_result [-Wunused-result]
180 | secp256k1_ec_pubkey_parse(CTX, &adaptor, adaptor33, 33);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I probably forgot to delete some unused variables. And ec_pubkey_parse returns a 0/1 value, I'll put it into a CHECK About the time_test warning, I forgot to change the code there and it might takes the original input format. |
Overview
This PR adds support for Schnorr Adaptor signatures. It is based on the work of Schnorr Signature by @jonasnick and @apoelstra, and the discussion #191.
Schnorr Adaptor Signatures
This implementation is mainly based on the idea of adaptor signatures in BIP-340 specification. It refers to the design and implementation of ECDSA adaptor signatures and MuSig2 adaptor signature.
Instead of having a verification algorithm for the adaptor signature, this module uses extract_t which extracts the adaptor point from the adaptor signature. (graph for complete atomic swap process)
Python
A Python specification of Schnorr Adaptor signatures has also been implemented, which generates all test vectors for the module.