Allow sign-to-contract commitments in schnorrsigs

jonasnick commented at 10:10 pm on January 29, 2019: contributor

This PR implements creation and verification of sign-to-contract commitments in the schnorrsig module. The following snippet for example puts a commitment to data32 into the (Schnorr) signature.

0/* Signer */
1secp256k1_s2c_commit_context s2c_ctx;
2secp256k1_s2c_commit_context_create(ctx, &s2c_ctx, data32);
3secp256k1_schnorrsig_sign(sign, &sig, &nonce_is_negated, msg, sk1, NULL, &s2c_ctx);
4secp256k1_s2c_commit_get_original_nonce(ctx, &s2c_original_nonce, &s2c_ctx);
5
6/* Verifier */
7secp256k1_schnorrsig_verify_s2c_commit(ctx, &sig, data32, &s2c_original_nonce, nonce_is_negated);

This PR is based on #558. The commits in this PR was previously included in #572. The tests should provide full coverage of the reachable lines.

There is also functionality for doing pay-to-contract style commitments (ec_commit), but it’s not exposed yet. If it will be in the future it would make sense to add an optional argument with a SHA midstate to allow hashing arbitrary version prefixes before hashing the public key (which would be useful in taproot for example).

Sign-to-contract and right now only work with schnorrsig_sign and nonce_function_bipschnorr. It should be straightforward to add support for ECDSA too in the future.

One thing to keep in mind is that for some users moving from ecdsa to schnorrsigs requires reading the documentation carefully. There is undocumented functionality in nonce_function_rfc6979 where the additional nonce data is hashed into the secret. If someone uses that and then call schnorrsig_sign with similar nonce data this results in a segmentation fault because nonce_function_bipschnorr which is used by default in schnorrsig_sign expects an s2c_commit_context object as nonce data and writes back to it.

add chacha20 function f4153a29ab

Add schnorrsig module which implements BIP-schnorr [0] compatible signing, verification and batch verification.

[0] https://github.com/sipa/bips/blob/bip-schnorr/bip-schnorr.mediawiki

d65adc82f8

Add secp256k1_context argument to secp256k1_nonce_functions 39fe790b89

jonasnick cross-referenced this on Jan 29, 2019 from issue Add anti nonce-sidechannel protocol to schnorrsigs by jonasnick

in src/secp256k1.c:614 in 72342d4fec outdated

609@@ -610,6 +610,103 @@ int secp256k1_ec_pubkey_combine(const secp256k1_context* ctx, secp256k1_pubkey *
610     return 1;
611 }
612 
613+/* Compute an ec commitment tweak as hash(pubkey, data). */
614+int secp256k1_ec_commit_tweak(const secp256k1_context *ctx, unsigned char *tweak32, const secp256k1_pubkey *pubkey, const unsigned char *data, size_t data_size) {

apoelstra commented at 10:55 pm on January 29, 2019:

Should be static

gmaxwell commented at 2:58 am on February 2, 2019: contributor

If you want to harden against misuse of the nonce function arguments, you could add a constant uint64 as the first member of the s2c_commit_context and test it.

Generally, I’d like to see us do that with all objects that we allow users to pass around: set a magic value when they’re constructed, zero it when they’re destroyed. Check for it before other accesses… exception being purely internal objects or elements where a caller might plausibly have thousands of them in existence at once (such that the overhead becomes a concern).

nonce_function_rfc6979 where the additional nonce data is hashed into the secret.

Odd that we didn’t document it– aux data is actually part of RFC6979.

in src/secp256k1.c:601 in 72342d4fec outdated

617+    size_t rbuf_size = sizeof(rbuf);
618+    secp256k1_sha256 sha;
619+
620+    if (data_size == 0) {
621+        /* That's probably not what the caller wanted */
622+        return 0;

real-or-random commented at 2:45 pm on February 4, 2019:

If that’s not what the caller wanted, maybe a CHECK or a VERIFY_CHECK is better.

jonasnick commented at 1:09 pm on February 9, 2019:

VERIFY_CHECK only works in tests. ARG_CHECK crashes the caller’s program. Not sure if that would be better given that there doesn’t really seem to be precedent for aborting in such cases in libsecp. Also I don’t see a good reason for aborting as this condition can be trivially recovered from (by just returning 0).

in src/tests.c:2361 in 72342d4fec outdated

2356+    secp256k1_rand256(seckey);
2357+    CHECK(secp256k1_ec_pubkey_create(ctx, &pubkey, seckey));
2358+    /* Create random data */
2359+    {
2360+        secp256k1_scalar d;
2361+        random_scalar_order_test(&d);

real-or-random commented at 2:57 pm on February 4, 2019:

secp256k1_rand256 instead?

jonasnick commented at 6:55 pm on February 11, 2019:

fixed

in src/secp256k1.c:685 in 72342d4fec outdated

701+    secp256k1_gej_set_infinity(&pj);
702+    secp256k1_pubkey_load(ctx, &p, &commitment_tmp);
703+    secp256k1_gej_add_ge(&pj, &pj, &p);
704+    secp256k1_pubkey_load(ctx, &p, commitment);
705+    secp256k1_ge_neg(&p, &p);
706+    secp256k1_gej_add_ge(&pj, &pj, &p);

real-or-random commented at 3:10 pm on February 4, 2019:

Maybe it’s a good idea to implement an gej_cmp or even pubkey_cmp… No idea.

jonasnick commented at 4:07 pm on February 11, 2019:

yeah, good idea. Maybe not in this PR.

in include/secp256k1.h:95 in 673d0e35a0 outdated

90+ *  or write to it directly. Use the secp256k1_s2c_commit_* instead to access a
91+ *  sign-to-contract context.
92+ *
93+ *  The exact representation of data inside is implementation defined and not
94+ *  guaranteed to be portable between different platforms or versions. It is however
95+ *  guaranteed to be 128 bytes in size, and can be safely copied/moved.

real-or-random commented at 3:34 pm on February 4, 2019:

one paragraph above, you say that copying is discouraged

jonasnick commented at 4:15 pm on February 11, 2019:

good catch. I think there’s no issue with copying that structure and I think I just copied this sentence from the MuSig session without thinking about it.

jonasnick commented at 6:55 pm on February 11, 2019:

fixed

in src/modules/schnorrsig/tests_impl.h:782 in 30c1d71311 outdated

775+        /* verify_s2c_commit fails if signature does not commit to data */
776+        secp256k1_schnorrsig sig_tmp;
777+        sig_tmp = sig;
778+        secp256k1_rand256(&sig_tmp.data[0]);
779+        CHECK(secp256k1_schnorrsig_verify_s2c_commit(ctx, &sig_tmp, data32, &s2c_original_nonce, nonce_is_negated) == 0);
780+    }

real-or-random commented at 3:44 pm on February 4, 2019:

there should be a test that verification fails when nonce_is_negated is wrong

jonasnick commented at 6:55 pm on February 11, 2019:

fixed

real-or-random commented at 3:47 pm on February 4, 2019: contributor

In general, I wonder whether it is a better idea

not to use the nonce function for s2c. (For example, this prevents people from using their own nonce function together with s2c.)
instead of a s2c context, have a s2c_opening, which contains all opening information (i.e., the original pubnonce and the information whether the nonce was negated. This looks more like a normal commitment API to me then.)

jonasnick commented at 7:03 pm on February 11, 2019: contributor

I added a commit that adds a magic number to the sign to contract context struct.

Add ec_commitments which are essentially the pay-to-contract-style tweaks of public keys.

The functionality is not exposed.

741ddd5232

Add and expose sign-to-contract contexts and make nonce_function_bipschnorr do sign-to-contract commitments if an s2c context is provided as nonce data 6ddb655985

Add verification of schnorrsig sign-to-contract commitments 8fa102cc29

jonasnick force-pushed on Feb 13, 2019

jonasnick commented at 1:47 pm on February 13, 2019: contributor

squashed

jonasnick cross-referenced this on Feb 14, 2019 from issue Allow sign-to-contract commitments in schnorrsigs [̶a̶l̶t̶e̶r̶n̶a̶t̶i̶v̶e̶]̶ by jonasnick

jonasnick cross-referenced this on Feb 15, 2019 from issue Add anti nonce-sidechannel protocol to schnorrsigs [̶a̶l̶t̶e̶r̶n̶a̶t̶i̶v̶e̶]̶ by jonasnick

in src/secp256k1.c:697 in 8fa102cc29

692+
693+    VERIFY_CHECK(ctx != NULL);
694+    ARG_CHECK(s2c_ctx != NULL);
695+    ARG_CHECK(data32 != NULL);
696+
697+    memcpy(s2c_ctx->magic, &s2c_commit_context_magic, sizeof(s2c_ctx->magic));

elichai commented at 3:41 pm on July 1, 2019:

Probably it’s just me being not familiar enough with C but why copy from a uint64_t to a unsigned char[8] instead of just putting a uint64_t in the struct?

jonasnick commented at 6:43 pm on July 1, 2019:

There was some reason I don’t remember to use uint64_t. But I think using a byte array in the struct is arbitrary… probably easier to read if replaced by uint64_t.

elichai commented at 10:02 pm on July 1, 2019:

Another thing, I’m pretty sure memcpy from uint64_t to unsigned char[8] is platform defined(BE/LE)

jonasnick commented at 7:18 pm on July 2, 2019:

Yes, but not really an issue as the doc clearly states

The exact representation of data inside is implementation defined and not guaranteed to be portable between different platforms or versions.

real-or-random commented at 11:08 pm on July 2, 2019:

It doesn’t matter in the end but the code is certainly easier to read if it’s the same type twice (no matter what type that is)

in src/secp256k1.c:761 in 8fa102cc29

756+        return 0;
757+    }
758+    if (data != NULL) {
759+        /* Create a sign-to-contract commitment by setting nonce32 <- nonce32 +
760+         * hash(nonce32*G, s2c_ctx->data) */
761+        secp256k1_s2c_commit_context *s2c_ctx = (secp256k1_s2c_commit_context *)data;

elichai commented at 4:03 pm on July 1, 2019:

I think you should verify the magic number here and fail if not right.

jonasnick commented at 6:39 pm on July 1, 2019:

yes, good catch

in include/secp256k1.h:104 in 8fa102cc29

 99+     * s2c_commit_contexts from a void pointer to check if they actually got an
100+     * s2c_commit_context and if it has been initialized. */
101+    unsigned char magic[8];
102+    unsigned char data[32];
103+    unsigned char data_hash[32];
104+    secp256k1_pubkey original_pubnonce;

elichai commented at 5:12 pm on July 1, 2019:

should the “public nonce” really be represented as pubkey? for me ge or gej would be more readable, but they’re probably not exported…. (I got confused while diving into secp256k1_ec_commit_seckey and secp256k1_ec_commit_tweak about the whole public key thing)

jonasnick commented at 6:41 pm on July 1, 2019:

ge or gej are not exported. But, hm, perhaps this should be a pubnonce type if it already confuses people.

real-or-random commented at 11:12 pm on July 2, 2019:

Hm, yes. A public key is a public key, and a nonce is a different thing. (I mean we can’t enforce anything in C but maybe it’s helpful to use different names for the two public types at least.)

Do we have the same problem in other places too?

real-or-random commented at 11:16 pm on July 2, 2019:

On a second thought… if we really want to optimize for a simple API, then #589 is the way to go. There the entire problem is eliminated by hiding from the user that a signature contains a “nonce”.

elichai commented at 11:23 pm on July 2, 2019:

The nonce is represented as a pubkey also there https://github.com/bitcoin-core/secp256k1/pull/589/files#diff-222d6d707e263c79c462fb223b0e3ddcR98

real-or-random commented at 11:32 pm on July 2, 2019:

okay, yes, but that struct is not really public: https://github.com/bitcoin-core/secp256k1/pull/589/files#diff-222d6d707e263c79c462fb223b0e3ddcR87

elichai commented at 11:43 pm on July 2, 2019:

fair

jonasnick commented at 12:59 pm on July 3, 2019: contributor

Closing in favor of #589. Thanks for the feedback.

jonasnick closed this on Jul 3, 2019

landanhu cross-referenced this on Dec 29, 2019 from issue Problem: rust-secp256k1 fork diverged from upstream by landanhu

Allow sign-to-contract commitments in schnorrsigs #588