Allow sign-to-contract commitments in schnorrsigs #588

jonasnick · 2019-01-29T22:10:25Z

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.

/* Signer */
secp256k1_s2c_commit_context s2c_ctx;
secp256k1_s2c_commit_context_create(ctx, &s2c_ctx, data32);
secp256k1_schnorrsig_sign(sign, &sig, &nonce_is_negated, msg, sk1, NULL, &s2c_ctx);
secp256k1_s2c_commit_get_original_nonce(ctx, &s2c_original_nonce, &s2c_ctx);

/* Verifier */
secp256k1_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.

…ning, verification and batch verification. [0] https://github.com/sipa/bips/blob/bip-schnorr/bip-schnorr.mediawiki

apoelstra · 2019-01-29T22:55:01Z

src/secp256k1.c

@@ -610,6 +610,103 @@ int secp256k1_ec_pubkey_combine(const secp256k1_context* ctx, secp256k1_pubkey *
 return 1;
 }

+/* Compute an ec commitment tweak as hash(pubkey, data). */
+int secp256k1_ec_commit_tweak(const secp256k1_context *ctx, unsigned char *tweak32, const secp256k1_pubkey *pubkey, const unsigned char *data, size_t data_size) {


Should be static

gmaxwell · 2019-02-02T02:58:32Z

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.

real-or-random

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.)

real-or-random · 2019-02-04T14:45:26Z

src/secp256k1.c

+
+ if (data_size == 0) {
+ /* That's probably not what the caller wanted */
+ return 0;


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

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).

real-or-random · 2019-02-04T14:57:26Z

src/tests.c

+ /* Create random data */
+ {
+ secp256k1_scalar d;
+ random_scalar_order_test(&d);


secp256k1_rand256 instead?

real-or-random · 2019-02-04T15:10:00Z

src/secp256k1.c

+ secp256k1_gej_add_ge(&pj, &pj, &p);
+ secp256k1_pubkey_load(ctx, &p, commitment);
+ secp256k1_ge_neg(&p, &p);
+ secp256k1_gej_add_ge(&pj, &pj, &p);


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

yeah, good idea. Maybe not in this PR.

real-or-random · 2019-02-04T15:34:31Z

include/secp256k1.h

+ *
+ * The exact representation of data inside is implementation defined and not
+ * guaranteed to be portable between different platforms or versions. It is however
+ * guaranteed to be 128 bytes in size, and can be safely copied/moved.


one paragraph above, you say that copying is discouraged

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.

real-or-random · 2019-02-04T15:44:59Z

src/modules/schnorrsig/tests_impl.h

+ sig_tmp = sig;
+ secp256k1_rand256(&sig_tmp.data[0]);
+ CHECK(secp256k1_schnorrsig_verify_s2c_commit(ctx, &sig_tmp, data32, &s2c_original_nonce, nonce_is_negated) == 0);
+ }


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

jonasnick · 2019-02-11T19:03:53Z

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

…eaks of public keys. The functionality is not exposed.

…chnorr do sign-to-contract commitments if an s2c context is provided as nonce data

jonasnick · 2019-02-13T13:47:27Z

squashed

elichai · 2019-07-01T15:41:21Z

src/secp256k1.c

+ ARG_CHECK(s2c_ctx != NULL);
+ ARG_CHECK(data32 != NULL);
+
+ memcpy(s2c_ctx->magic, &s2c_commit_context_magic, sizeof(s2c_ctx->magic));


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?

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.

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

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.

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)

elichai · 2019-07-01T16:03:20Z

src/secp256k1.c

+ if (data != NULL) {
+ /* Create a sign-to-contract commitment by setting nonce32 <- nonce32 +
+ * hash(nonce32*G, s2c_ctx->data) */
+ secp256k1_s2c_commit_context *s2c_ctx = (secp256k1_s2c_commit_context *)data;


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

yes, good catch

elichai · 2019-07-01T17:12:34Z

include/secp256k1.h

+ unsigned char magic[8];
+ unsigned char data[32];
+ unsigned char data_hash[32];
+ secp256k1_pubkey original_pubnonce;


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)

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

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?

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".

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

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

jonasnick · 2019-07-03T12:59:33Z

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

apoelstra and others added 3 commits January 29, 2019 10:17

add chacha20 function

f4153a2

Add schnorrsig module which implements BIP-schnorr [0] compatible sig…

d65adc8

…ning, verification and batch verification. [0] https://github.com/sipa/bips/blob/bip-schnorr/bip-schnorr.mediawiki

Add secp256k1_context argument to secp256k1_nonce_functions

39fe790

jonasnick mentioned this pull request Jan 29, 2019

Add anti nonce-sidechannel protocol to schnorrsigs #572

Closed

apoelstra reviewed Jan 29, 2019

View reviewed changes

real-or-random reviewed Feb 4, 2019

View reviewed changes

jonasnick added 3 commits February 13, 2019 13:45

Add ec_commitments which are essentially the pay-to-contract-style tw…

741ddd5

…eaks of public keys. The functionality is not exposed.

Add and expose sign-to-contract contexts and make nonce_function_bips…

6ddb655

…chnorr do sign-to-contract commitments if an s2c context is provided as nonce data

Add verification of schnorrsig sign-to-contract commitments

8fa102c

jonasnick force-pushed the schnorrsig-s2c-only branch from e726610 to 8fa102c Compare February 13, 2019 13:47

This was referenced Feb 14, 2019

Allow sign-to-contract commitments in schnorrsigs [̶a̶l̶t̶e̶r̶n̶a̶t̶i̶v̶e̶]̶ #589

Closed

Add anti nonce-sidechannel protocol to schnorrsigs [̶a̶l̶t̶e̶r̶n̶a̶t̶i̶v̶e̶]̶ #590

Closed

elichai reviewed Jul 1, 2019

View reviewed changes

jonasnick closed this Jul 3, 2019

landanhu mentioned this pull request Dec 29, 2019

Problem: rust-secp256k1 fork diverged from upstream crypto-com/thaler#757

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow sign-to-contract commitments in schnorrsigs #588

Allow sign-to-contract commitments in schnorrsigs #588

jonasnick commented Jan 29, 2019 •

edited

Loading

apoelstra Jan 29, 2019

gmaxwell commented Feb 2, 2019 •

edited

Loading

real-or-random left a comment

real-or-random Feb 4, 2019

jonasnick Feb 9, 2019

real-or-random Feb 4, 2019

jonasnick Feb 11, 2019

real-or-random Feb 4, 2019

jonasnick Feb 11, 2019

real-or-random Feb 4, 2019

jonasnick Feb 11, 2019

jonasnick Feb 11, 2019

real-or-random Feb 4, 2019

jonasnick Feb 11, 2019

jonasnick commented Feb 11, 2019

jonasnick commented Feb 13, 2019

elichai Jul 1, 2019 •

edited

Loading

jonasnick Jul 1, 2019

elichai Jul 1, 2019

jonasnick Jul 2, 2019

real-or-random Jul 2, 2019

elichai Jul 1, 2019 •

edited

Loading

jonasnick Jul 1, 2019

elichai Jul 1, 2019

jonasnick Jul 1, 2019

real-or-random Jul 2, 2019

real-or-random Jul 2, 2019

elichai Jul 2, 2019 •

edited

Loading

real-or-random Jul 2, 2019 •

edited

Loading

elichai Jul 2, 2019

jonasnick commented Jul 3, 2019 •

edited

Loading

Allow sign-to-contract commitments in schnorrsigs #588

Allow sign-to-contract commitments in schnorrsigs #588

Conversation

jonasnick commented Jan 29, 2019 • edited Loading

Choose a reason for hiding this comment

gmaxwell commented Feb 2, 2019 • edited Loading

real-or-random left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasnick commented Feb 11, 2019

jonasnick commented Feb 13, 2019

elichai Jul 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elichai Jul 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elichai Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

real-or-random Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasnick commented Jul 3, 2019 • edited Loading

jonasnick commented Jan 29, 2019 •

edited

Loading

gmaxwell commented Feb 2, 2019 •

edited

Loading

elichai Jul 1, 2019 •

edited

Loading

elichai Jul 1, 2019 •

edited

Loading

elichai Jul 2, 2019 •

edited

Loading

real-or-random Jul 2, 2019 •

edited

Loading

jonasnick commented Jul 3, 2019 •

edited

Loading