Extract the secret key from a keypair #845

elichai commented at 2:52 PM on November 5, 2020: contributor

With schnorrsig if you need to tweak the secret key (for BIP32) you must use the keypair API to get compatible secret/public keys which you do by calling secp256k1_keypair_xonly_tweak_add(), but after that there's no currently a way to extract the secret key back for storage. so I added a secp256k1_keypair_seckey function to extract the key

in src/modules/extrakeys/main_impl.h:195 in 6199885504 outdated

 190 | +    VERIFY_CHECK(ctx != NULL);
 191 | +    ARG_CHECK(seckey32 != NULL);
 192 | +    memset(seckey32, 0, 32);
 193 | +    ARG_CHECK(keypair != NULL);
 194 | +
 195 | +    memcpy(seckey32, &keypair->data[0], 32);

jonasnick commented at 9:37 PM on November 5, 2020:

You may want to use secp256k1_keypair_seckey_load instead of memcpy because it correctly handles zeroed keypairs.

elichai commented at 10:27 PM on November 5, 2020:

I can change it, I've used memcpy because just underneath the function that extracts a pubkey just does a mempy: https://github.com/bitcoin-core/secp256k1/blob/e89278f211a526062745c391d48a7baf782b4b2b/src/modules/extrakeys/main_impl.h#L195

jonasnick commented at 8:59 AM on November 9, 2020:

I see. Yeah, it should be consistent with the pubkey getter. Fine for now imo, since this outputs an invalid seckey, we don't mention a guarantee that a function fails if called with an invalid keypair and the current version is simpler than the alternative.

jonasnick commented at 9:41 PM on November 5, 2020: contributor

Concept ACK

Usually we test the functions more exhaustively - see for example the keypair_pub tests. We should have tests where the non-ctx arguments are NULL and check that the seckey is zeroed.

elichai force-pushed on Nov 7, 2020

jonasnick commented at 9:00 AM on November 9, 2020: contributor

ACK 095f5eaa97dc639a8415cc52f640dafe37b4c245

real-or-random commented at 2:45 PM on November 27, 2020: contributor

Concept ACK

I think this function should be added to the constant-time tests.

edit: And a nit on the naming: The other function is just ..._pub, so should this be just ..._sec? Not sure to be honest.

elichai commented at 10:26 AM on November 30, 2020: contributor

edit: And a nit on the naming: The other function is just ..._pub, so should this be just ..._sec? Not sure to be honest.

hmm I'm also not sure if it's better to stay "consistent" with the half name or write the "full name", I'll change this if anyone prefers a specific side

real-or-random commented at 10:31 AM on November 30, 2020: contributor

@jonasnick Was there a specific reason you choose the half name?

(I know it's kind of annoying to have these debates but I think it's better to have them early than never.)

jonasnick commented at 2:06 PM on November 30, 2020: contributor

@jonasnick Was there a specific reason you choose the half name?

Yes, the name's in the module are generally too long already and key is already part of the name. _sec doesn't sound as nice, could also call it _secret. But I'd (slightly) prefer to be consistent.

sipa commented at 6:19 PM on December 18, 2020: contributor

Concept ACK. Adding to constant-time tests would be good.

elichai force-pushed on Dec 19, 2020

Add a function to extract the secretkey from a keypair fc96aa73f5

Add seckey extraction from keypair to the extrakeys tests 36d9dc1e8e

elichai force-pushed on Dec 19, 2020

Add secret key extraction from keypair to constant time tests 33cb3c2b1f

elichai force-pushed on Dec 19, 2020

elichai commented at 9:07 AM on December 19, 2020: contributor

@sipa @jonasnick Fixed

jonasnick commented at 2:21 PM on December 21, 2020: contributor

ACK 33cb3c2b1fc3f3fe46c6d0eab118248ea86c1f06

elichai cross-referenced this on Jan 12, 2021 from issue Rebased version of `[Alternative] Allow deserializing from owned types` + support for new schnorr module by thomaseizinger

real-or-random approved

real-or-random commented at 9:54 AM on January 12, 2021: contributor

ACK 33cb3c2b1fc3f3fe46c6d0eab118248ea86c1f06 code inspection, tests pass

real-or-random merged this on Jan 12, 2021

real-or-random closed this on Jan 12, 2021

elichai deleted the branch on Jan 12, 2021

elichai cross-referenced this on Jan 12, 2021 from issue Replace seckey extraction from keypair by elichai

Fabcien referenced this in commit 01ee062811 on Apr 8, 2021

deadalnix referenced this in commit 49a9bd8bf0 on Apr 9, 2021

elichai cross-referenced this on May 10, 2021 from issue KeyPair serialization by dr-orlovsky