[API CHANGE] Introduce pubkey and signature types #282

sipa · 2015-07-20T18:47:22Z

This introduces a new data type secp256k1_pubkey_t, which is an abstract parsed public key, convertible to/from serialized variable-size public keys.

This avoids all API problems resulting from needing to pass pubkey sizes and pointers to pubkey sizes anywhere public keys are needed.

Advantages:

No need for secp256k1_ec_pubkey_verify, secp256k1_ec_pubkey_compress, secp256k1_ec_pubkey_decompress
Reusing the same public key in multiple operations is faster due to avoiding decompression costs on every use.
Less parameters to pass around.
No need for separate secp256k1_ecdsa_verify return code to deal with invalid public keys.

Disadvantages:

Need for API calls secp256k1_ec_pubkey_serialize and secp256k1_ec_pubkey_parse.
Slightly slower for verification (0.4%) due to extra serialization and weak validation overhead between parsing and verifying.
Potentially unsafe when passing an uninitialized secp256k1_pubkey_t for verification, though this is mitigated as much as possible by wiping the output secp256k1_pubkey_t any time an invalid one would be returned, and not accepting a wiped one as input.

sipa · 2015-07-20T19:23:07Z

An alternative to this is having a secp256k1_pubkey_t type that maintains the compressedness, and is just a serialized pubkey, but in a constant-size buffer.

This has as advantages that it does not add any significant performance overhead, and does not risk extra problems when uninitialized secp256k1_pubkey_t's are passed in (though that would still be a risky bug for other reasons), but it also would not avoid the compress/decompress/verify API calls, not offer any performance advantages for repeated pubkey use, and be a bit unintuitive for having parsing of an invalid pubkey potentially succeed.

sipa · 2015-07-23T16:42:32Z

@afk11 Any comments here? This significantly changes the way public keys are handled, and removes the need for the secp256k1_ec_pubkey_compress function which you added.

sipa · 2015-07-24T12:59:58Z

Added extra documentation.

apoelstra · 2015-07-24T19:09:02Z

src/secp256k1.c

+    secp256k1_fe_get_b32(pubkey->data + 32, &ge->y);
+}
+
+int secp256k1_ec_pubkey_parse(const secp256k1_context_t* ctx, secp256k1_pubkey_t* pubkey, const unsigned char *input, int inputlen) {


The indentation in this function is different from all the others.

apoelstra · 2015-07-24T19:17:52Z

It looks like bench_recover fails:

[apoelstra@titanic secp256k1]$ ./bench_recover 
src/bench_recover.c:26: test condition failed: secp256k1_ecdsa_recover_compact(data->ctx, data->msg, data->sig, &pubkey, 1)
Aborted

DavidEGrayson · 2015-07-24T21:07:55Z

I was expecting @gmaxwell to point out that _t is reserved by POSIX (see #198).

sipa · 2015-07-25T15:29:08Z

@DavidEGrayson I thought I already answered, but can't see the post now

@gmaxwell would be right to point that out, but then we should consistently change it throughout the codebase once, not introduce multiple coding standards.

sipa · 2015-07-25T15:30:00Z

I've changed the implementation of secp256k1_pubkey_load/save now, using secp256k1_ge_storage where possible... and the result is around 0.15% faster now than before the pubkey_t introduction.

sipa · 2015-07-25T23:55:21Z

I extended the scope of this patch, and introduced a secp256k1_ecdsa_signature_t for ECDSA signatures.

apoelstra · 2015-07-26T13:34:10Z

include/secp256k1.h

- *           0: incorrect signature
- *          -1: invalid public key
- *          -2: invalid signature
+ *           0: incorrect or unparseable signature
 * In:       ctx:       a secp256k1 context object, initialized for verification.
 *           msg32:     the 32-byte message hash being verified (cannot be NULL)
 *           sig:       the signature being verified (cannot be NULL)
 *           siglen:    the length of the signature


Update comment for removal of siglen

apoelstra · 2015-07-26T13:38:31Z

include/secp256k1.h

 /** Compute the public key for a secret key.
 *  In:     ctx:        pointer to a context object, initialized for signing (cannot be NULL)
 *          compressed: whether the computed public key should be compressed
 *          seckey:     pointer to a 32-byte private key (cannot be NULL)
- *  Out:    pubkey:     pointer to a 33-byte (if compressed) or 65-byte (if uncompressed)
+ *  Out:    pubkey:     pointer to the created public key (cannot be NULL)
 *                      area to store the public key (cannot be NULL)
 *          pubkeylen:  pointer to int that will be updated to contains the pubkey's
 *                      length (cannot be NULL)


Remove second line under pubkey, entry for pubkeylen, entry for compressed in this comment

apoelstra · 2015-07-26T13:48:50Z

src/secp256k1.c

@@ -54,11 +54,152 @@ void secp256k1_context_destroy(secp256k1_context_t* ctx) {
    free(ctx);
 }

-int secp256k1_ecdsa_verify(const secp256k1_context_t* ctx, const unsigned char *msg32, const unsigned char *sig, int siglen, const unsigned char *pubkey, int pubkeylen) {
+static void secp256k1_pubkey_load(secp256k1_ge_t* ge, const secp256k1_pubkey_t* pubkey) {
+    if (sizeof(secp256k1_ge_storage_t) == 64) {


Under what circumstances is this not 64? You are depending on its exact format in the case that it is, so I'm confused about what you think could change.

Who knows what a compiler can do with integer sizes and alignments. Maybe we want to add debug fields at some point, when VERIFY is enabled.

It's also not relying on its exact representation, only that it can be memcpy'd to and from a 64-byte array, which is faster than going through the b32 representation.

In any case, this probably requires some comments in the code.

Yeah, I realized that you weren't depending on the representation down below when I saw you were doing the same thing with scalar_t. I think you should add a comment saying that (assuming the data fits) the API only allows the content of pubkey->data to be a memcpy of a preexisting secp256k1_ge_storage_t, so this is fine.

sipa · 2015-07-26T14:03:17Z

Added several comments about the representation of pubkey and signature types.

apoelstra · 2015-07-26T14:21:47Z

I find it a little confusing to have two signature types, especially ones that are so similarly named. It's hard to remember which one is the public one and which is the internal.

The internal one, secp256k1_ecdsa_sig_t, consists of an (r, s) pair, and is used only by the five functions defined in ecdsa.h. If you grep for these five functions you will see that every one of them occurs in only two places: in their passthroughs in the public API, and in the unit tests.

I propose:

Dropping secp256k1_ecdsa_sig_t and having the ecdsa.h functions take a public secp256k1_ecdsa_signature_t.
Moving signature_load and signature_save into ecdsa_impl.h, and having them output separate r, s scalars rather than a single struct (since the ecdsa.h functions use the individual scalars and never pass around the complete type anyway).

This would avoid having separate types and remove some double conversions (like calling secp256k1_ecdsa_sig_parse immediately followed by secp256k1_ecdsa_signature_save in secp256k1_ecdsa_signature_parse_der). Also note that recid is already carried around separately from secp256k1_ecdsa_sig_t, so this type is not doing a great job of encapsulation anyway.

I don't feel super strongly about this, it's up to you.

sipa · 2015-07-26T14:28:53Z

I think that would be a layering violation. the toplevel module defines a data type for external use, and depends on the internal code for operations on it. The lowlevel module would end up depending on the toplevel module if it needed to be passed in the toplevel data type, resulting in a weak circular dependency.

Furthermore, both datatypes have a different use case: one is obscurity and consistency (at least wrt its size), the other is efficiency.

Moving the load and save to ecdsa feels wrong, as it makes the ecdsa module depend on external API peculiarities.

One option is to drop the internal type, and just operate on scalar_t's inside ecdsa. The other is perhaps to rename the internal types to something more obvious (secp256k1_ecdsa_signature_internal_t ?), and maybe move the recid inside it.

apoelstra · 2015-07-26T14:31:53Z

That's a good point.

Between the two options you listed I'd prefer to just operate on scalar_t's, since there are only two of them and they have standard names.

afk11 · 2015-07-26T14:38:57Z

@sipa concept ACK on this, the performance boost should affect me also since presently I have to serialize my PublicKey types whenever calling secp256k1_* functions. I'll get to testing this today.

sipa · 2015-07-26T14:53:02Z

@apoelstra Added a commit which removes the internal secp256k1_ecdsa_sig_t.

apoelstra · 2015-07-26T14:57:27Z

Thanks, I'm much happier with this.

apoelstra · 2015-07-26T15:25:48Z

ACK

18c329c Remove the internal secp256k1_ecdsa_sig_t type (Pieter Wuille) 74a2acd Add a secp256k1_ecdsa_signature_t type (Pieter Wuille) 23cfa91 Introduce secp256k1_pubkey_t type (Pieter Wuille)

sipa changed the title ~~Introduce secp256k1_pubkey_t type~~ [API CHANGE] Introduce secp256k1_pubkey_t type Jul 20, 2015

sipa force-pushed the pubkeytype branch from f8e7627 to d253a63 Compare July 20, 2015 18:54

sipa force-pushed the pubkeytype branch from d253a63 to 24ae07c Compare July 24, 2015 12:54

sipa mentioned this pull request Jul 24, 2015

Implement Schnorr signatures #212

Merged

apoelstra reviewed Jul 24, 2015
View reviewed changes

sipa force-pushed the pubkeytype branch 2 times, most recently from c2f173c to e71b1f4 Compare July 24, 2015 19:43

sipa force-pushed the pubkeytype branch from e71b1f4 to 2e9d6f1 Compare July 24, 2015 21:46

sipa mentioned this pull request Jul 24, 2015

Introduce callback functions for dealing with errors #278

Merged

sipa force-pushed the pubkeytype branch from 2e9d6f1 to 1353ba1 Compare July 25, 2015 15:27

sipa changed the title ~~[API CHANGE] Introduce secp256k1_pubkey_t type~~ [API CHANGE] Introduce pubkey and signature types Jul 25, 2015

sipa force-pushed the pubkeytype branch from fa6bdbb to 3e9aff1 Compare July 26, 2015 00:38

apoelstra reviewed Jul 26, 2015
View reviewed changes

sipa force-pushed the pubkeytype branch from 3e9aff1 to 636a31e Compare July 26, 2015 13:35

apoelstra reviewed Jul 26, 2015
View reviewed changes

sipa force-pushed the pubkeytype branch from 636a31e to 70f1070 Compare July 26, 2015 13:40

apoelstra reviewed Jul 26, 2015
View reviewed changes

sipa added 2 commits July 26, 2015 15:59

Introduce secp256k1_pubkey_t type

23cfa91

Add a secp256k1_ecdsa_signature_t type

74a2acd

sipa force-pushed the pubkeytype branch from 70f1070 to 74a2acd Compare July 26, 2015 14:02

Remove the internal secp256k1_ecdsa_sig_t type

18c329c

sipa merged commit 18c329c into bitcoin-core:master Jul 26, 2015

[API CHANGE] Introduce pubkey and signature types #282

[API CHANGE] Introduce pubkey and signature types #282

Uh oh!

Conversation

sipa commented Jul 20, 2015

Uh oh!

sipa commented Jul 20, 2015

Uh oh!

sipa commented Jul 23, 2015

Uh oh!

sipa commented Jul 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Jul 24, 2015

Uh oh!

DavidEGrayson commented Jul 24, 2015

Uh oh!

sipa commented Jul 25, 2015

Uh oh!

sipa commented Jul 25, 2015

Uh oh!

sipa commented Jul 25, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Jul 26, 2015

Uh oh!

apoelstra commented Jul 26, 2015

Uh oh!

sipa commented Jul 26, 2015

Uh oh!

apoelstra commented Jul 26, 2015

Uh oh!

afk11 commented Jul 26, 2015

Uh oh!

sipa commented Jul 26, 2015

Uh oh!

apoelstra commented Jul 26, 2015

Uh oh!

apoelstra commented Jul 26, 2015

Uh oh!

Uh oh!