Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Extract the secret key from a keypair #845

Merged
merged 3 commits into from
Jan 12, 2021
Merged

Extract the secret key from a keypair #845

merged 3 commits into from
Jan 12, 2021

Conversation

elichai
Copy link
Contributor

@elichai elichai commented Nov 5, 2020

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

jonasnick
jonasnick reviewed Nov 5, 2020
View reviewed changes
Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/modules/extrakeys/main_impl.h Outdated Show resolved Hide resolved
jonasnick
jonasnick reviewed Nov 9, 2020
View reviewed changes
Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 095f5ea

@real-or-random
Copy link
Contributor

real-or-random commented Nov 27, 2020 •
edited
Loading

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
Copy link
Contributor Author

elichai commented Nov 30, 2020

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
Copy link
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
Copy link
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
Copy link
Contributor

sipa commented Dec 18, 2020

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

@elichai
Copy link
Contributor Author

elichai commented Dec 19, 2020

@sipa @jonasnick Fixed

jonasnick
jonasnick reviewed Dec 21, 2020
View reviewed changes
Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 33cb3c2

@elichai elichai mentioned this pull request Jan 12, 2021
Merged
real-or-random
real-or-random approved these changes Jan 12, 2021
View reviewed changes
Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 33cb3c2 code inspection, tests pass

@real-or-random real-or-random merged commit 328aaef into bitcoin-core:master Jan 12, 2021
@elichai elichai deleted the extrakeys-seckey branch January 12, 2021 11:10
@elichai elichai mentioned this pull request Jan 12, 2021
Closed
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Apr 8, 2021
@elichai @Fabcien
[SECP256K1] Extract the secret key from a keypair
01ee062 
Summary:
```
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
```

Backport of [[bitcoin-core/secp256k1#845 | secp256k1#845]]

Test Plan:
  ninja check-secp256k1

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D9381
deadalnix pushed a commit to Bitcoin-ABC/secp256k1 that referenced this pull request Apr 9, 2021
@elichai @Fabcien
[SECP256K1] Extract the secret key from a keypair
49a9bd8 
Summary:
```
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
```

Backport of [[bitcoin-core/secp256k1#845 | secp256k1#845]]

Test Plan:
  ninja check-secp256k1

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D9381
@elichai elichai mentioned this pull request May 10, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonasnick jonasnick jonasnick left review comments

@real-or-random real-or-random real-or-random approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@elichai @real-or-random @jonasnick @sipa