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.

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

PrivateKey should have methods to derive addresses more easily #1865

Open
Kixunil opened this issue May 19, 2023 · 12 comments
Open

PrivateKey should have methods to derive addresses more easily #1865

Kixunil opened this issue May 19, 2023 · 12 comments
Labels
API hole Important API missing - significantly degrades usability or idiomatic usage

Comments

@Kixunil
Copy link
Collaborator

Kixunil commented May 19, 2023

Currently, if you want to derive p2tr you need to write:

let pub_key = private_key.inner.x_only_public_key(&**secp256k1::SECP256K1).0;
Address::p2tr(&secp256k1::SECP256K1, pub_key, None, private_key.network)

This is pretty hard to discover for newbies - the .inner and .0 is annoying, there's SECP256K1 stuff, you need to sync networks...
Having .to_p2tr_address() and other methods would make it much easier to use.

@Kixunil Kixunil added the API hole Important API missing - significantly degrades usability or idiomatic usage label May 19, 2023
@tcharding
Copy link
Member

I spent some clock cycles this week thinking about the various keys/address APIs, and how we could improve them. This issue made me chuckle.

@stevenroose
Copy link
Collaborator

I disagree with direct .to_xxx_address getters, but yeah privkey -> pubkey has always been annoying, but I thought the bitcoin::PrivateKey already made that better than secp256k1::SecretKey (where the only way was secp256k1::PublicKey::from_secret_key(&SECP, secret_key). I guess just adding a .to_xonly_pubkey to PrivateKey should be sufficient, no? I think we can drop the parity part too in a method like that. (And maybe have a _with_parity extra one to expose that).

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jun 5, 2023

@stevenroose the reason to go directly to address is to transfer network. We would either have to add it to PublicKey or force people to copy it themselves which adds a small possibility of error.

@junderw
Copy link
Contributor

junderw commented Jun 5, 2023

Just throwing in my 2 cents:

Out of my many years working on and with bitcoinjs-lib in my day job... the amount of times I have written code where I want to go from a private key to an address are is zero.

Especially in a post-BIP32 world, we work with xpubs almost exclusively for address derivation.

Our seed mnemonics only ever touch code that is meant for signing.

Not necessarily NACKing this, but I don't see the utility... maybe a more concrete use case would make things clearer?

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jun 6, 2023

@junderw the xpub API in this library goes through PrivateKey, I wouldn't have suggested it otherwise.

@stevenroose
Copy link
Collaborator

@junderw the xpub API in this library goes through PrivateKey, I wouldn't have suggested it otherwise.

@Kixunil that's not really true. That's only the private part, the xpriv. But wallets will more likely store xpubs, so from there you derive PublicKeys that are network-less and then you go from publickey to address.

I agree with @junderw that sk->addr is very uncommon. Even in hal where might be one of the few cases where that happens (because it's a utility), I first do privkey info, then pubkey info and then address info from the pubkey info. Like the privkey->addr step doesn't make sense without going through the pubkey step. And IMO that has nothing to do with the API.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jun 12, 2023

Ah, good point about xpubs. I wonder if PublicKey should contain Network then.

@apoelstra
Copy link
Member

Ah, good point about xpubs. I wonder if PublicKey should contain Network then.

It could, but then we couldn't parse one from a script or a string.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jun 22, 2023

Oh, true. So the xpub API should be improved somehow. Will try to think of something later.

@stevenroose
Copy link
Collaborator

Yeah no network in pubkey please, really doesn't make sense. It's bad enough we have a "privatekey" struct that has a network and compression.

Over the years I made multiple attempts to get rid of bitcoin::PrivateKey and bitcoin::PublicKey and just have a wif::Key or something for WIF format keys.

None really worked out easily, but I personally avoid the bitcoin::*Key* types as much as I can and always use the secp256k1::*Key* types.

This week been doing some taproot stuff and I noticed there are various new key types all over the place. XOnly, KeyPair, Untweaked, Tweaked, ...

@apoelstra
Copy link
Member

I exclusively use bitcoin::PublicKey and try to avaid secp256k1::PublicKey because it is missing compressedness information, which does actually feed into what addresses are derived etc. But I am very glad that for x-only keys, the bitcoin and secp versions are the same type.

I'm not sure I've ever used privkeys in non-toy code, and even in toy code I try to avoid it, so I don't have much of an opinion there.

@stevenroose
Copy link
Collaborator

I exclusively use bitcoin::PublicKey and try to avaid secp256k1::PublicKey because it is missing compressedness information, which does actually feed into what addresses are derived etc. But I am very glad that for x-only keys, the bitcoin and secp versions are the same type.

I'm not sure I've ever used privkeys in non-toy code, and even in toy code I try to avoid it, so I don't have much of an opinion there.

Hehe, I do the exact opposite. I try to avoid bitcoin::PublicKey at all times and annoyedly wrap my pubkeys with it whenever some rust-bitcoin API requires it. I don't think I have ever touched a non-compressed public key in my life.

IMO we'd be better of having some kind of utils::weird::wtheven::howlonghaveyoubeenasleep::uncompressed module where all the methods you'd need to use uncompressed keys live so that we can stop worrying about it at all in all other code.

Like, while I was doing the MR of removing bitcoin::PublicKey, the literal only place where I ran into some trouble was the PSBT partial_sigs field. Ironincally, PSBT is 2 years younger than segwit where uncompressed keys were basically deprecated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API hole Important API missing - significantly degrades usability or idiomatic usage
Projects
None yet
Development

No branches or pull requests

5 participants