Add secp256k1_ec_pubkey_compress() #263

afk11 commented at 1:34 PM on June 26, 2015: contributor

Added general function to reserialize a public key, and added secp256k1_ec_pubkey_compress() after seeing https://github.com/bitcoin/secp256k1/issues/259

afk11 commented at 8:58 PM on June 26, 2015: contributor

I'm not yet sure why travis failed, it was a seemingly unrelated check. Will take another look.

afk11 commented at 6:55 PM on July 5, 2015: contributor

Ok - nothing major here, just added a function that does the opposite of secp256k1_ec_pubkey_decompress() Ended up removing the new function in eckey_impl.h - it wasn't really worth it as the code is just a few lines.

Anyways, hope this proves useful enough to merge. I wouldn't push C code too often - any feedback/criticism would be welcome.

I added a test also, similar to how decompress is tested here: https://github.com/bitcoin/secp256k1/pull/263/files#diff-4655d106bf03045a3a50beefc800db21R1521

in src/eckey.h:None in 8633317d00 outdated

  13 | @@ -14,6 +14,7 @@
  14 |  
  15 |  static int secp256k1_eckey_pubkey_parse(secp256k1_ge_t *elem, const unsigned char *pub, int size);
  16 |  static int secp256k1_eckey_pubkey_serialize(secp256k1_ge_t *elem, unsigned char *pub, int *size, int compressed);
  17 | +static int secp256k1_eckey_pubkey_reserialize( const unsigned char *pub, unsigned char *pubkeyout, int *size, int compressed);

sipa commented at 12:21 PM on July 6, 2015:

This function does not seem to exist.

afk11 commented at 1:37 PM on July 6, 2015:

Thanks, removed it now.

sipa commented at 12:37 PM on July 6, 2015: contributor

Please squash your last commit into the previous ones. All commits should result in a tree that passes tests, or we may break the ability to bisect.

afk11 force-pushed on Jul 6, 2015

afk11 commented at 3:29 PM on July 6, 2015: contributor

Ok, just pushed them once more.

in src/eckey.h:None in fead8c19c8 outdated

  23 | @@ -24,3 +24,4 @@ static int secp256k1_eckey_privkey_tweak_mul(secp256k1_scalar_t *key, const secp
  24 |  static int secp256k1_eckey_pubkey_tweak_mul(const secp256k1_ecmult_context_t *ctx, secp256k1_ge_t *key, const secp256k1_scalar_t *tweak);
  25 |  
  26 |  #endif
  27 | +

apoelstra commented at 6:30 PM on July 7, 2015:

Can you rebase without this newline?

Normally I wouldn't complain but this is the only place you touch this file.

afk11 commented at 10:04 PM on July 8, 2015:

Woops - hadn't spotted that, thanks.

in src/secp256k1.c:None in fead8c19c8 outdated

 432 | @@ -418,3 +433,4 @@ int secp256k1_context_randomize(secp256k1_context_t* ctx, const unsigned char *s
 433 |      secp256k1_ecmult_gen_blind(&ctx->ecmult_gen_ctx, seed32);
 434 |      return 1;
 435 |  }
 436 | +

apoelstra commented at 6:30 PM on July 7, 2015:

And this one, while you're rebasing anyway.

in include/secp256k1.h:None in fead8c19c8 outdated

 259 | @@ -260,6 +260,23 @@ SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_pubkey_create(
 260 |    int compressed
 261 |  ) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4);
 262 |  
 263 | +/** Compress a public key.
 264 | + * In:     ctx:       pointer to a context object (cannot be NULL)
 265 | + * In:     pubkeyin:  pointer to a 33-byte or 65-byte public key (cannot be NULL)

apoelstra commented at 6:32 PM on July 7, 2015:

Typo: drop second In:

in src/secp256k1.c:None in fead8c19c8 outdated

 292 | +    DEBUG_CHECK(pubkeyout != NULL);
 293 | +    DEBUG_CHECK(pubkeylen != NULL);
 294 | +    (void)ctx;
 295 | +
 296 | +    if (secp256k1_eckey_pubkey_parse(&p, pubkeyin, *pubkeylen)) {
 297 | +        ret = secp256k1_eckey_pubkey_serialize(&p, pubkeyout, pubkeylen, 1);

apoelstra commented at 6:33 PM on July 7, 2015:

I wonder if we should VERIFY_CHECK this instead of returning the return value. Is there a way this can fail if the parsing was successful?

sipa commented at 9:09 PM on July 8, 2015:

Nope.

sipa commented at 9:10 PM on July 8, 2015: contributor

ACK, though addressing @apoelstra's nits would be nice.

afk11 force-pushed on Jul 8, 2015

afk11 commented at 10:04 PM on July 8, 2015: contributor

Cool, rebased and addressed the above!

apoelstra commented at 10:41 PM on July 8, 2015: contributor

Tested ACK from me. (The VERIFY_CHECK thing is no big deal.)

Add secp256k1_ec_pubkey_compress(), with test similar to the related decompress() function. 99fd963bd5

in include/secp256k1.h:None in 8671d19243 outdated

 259 | @@ -260,9 +260,26 @@ SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_pubkey_create(
 260 |    int compressed
 261 |  ) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4);
 262 |  
 263 | +/** Compress a public key.
 264 | + * In:     ctx:       pointer to a context object (cannot be NULL)
 265 | + *         pubkeyin:  pointer to a 33-byte or 65-byte public key (cannot be NULL)
 266 | + * In/Out: pubkeyout: pointer to a 33-byte array to put the compressed public key (cannot be NULL)

sipa commented at 4:17 PM on July 10, 2015:

pubkeyout is Out only, no?

afk11 commented at 4:55 PM on July 10, 2015:

You're right. Could still be the same for decompress as well, I'll take a look now.

afk11 force-pushed on Jul 13, 2015

sipa merged this on Jul 14, 2015

sipa closed this on Jul 14, 2015

sipa referenced this in commit 4fb174df08 on Jul 14, 2015