-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
BIP 178: Version Extended WIF #673
Conversation
Changing this to a Concept ACK for now, as there may also be a good argument to switch to a deliberately incompatible WIF format for the new key types: because other implementations handle non-1 suffix bytes in different ways. They don't all explicitly reject them, or even treat them as compressed. For example bitcoin-python https://github.com/petertodd/python-bitcoinlib/blob/master/bitcoin/wallet.py#L253 treats only keys with suffix byte AND suffix byte=1 as compressed and the rest as uncompressed. @petertodd Not sure how much of a problem this is, but it seems like an edge case that might result in fund loss (or temporary panic at least...). |
(It may be desirable to change Title?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
b5b7c71
to
a703490
Compare
d61bdb0
to
9f21bfd
Compare
@luke-jr You mean "Typed Private Keys"? It seemed good enough to me, but maybe "Extended Wallet Import Format"? |
0158061
to
75dd9a1
Compare
An alternative to this approach is to use bech32 for the private keys as well, i.e. switch to a whole new format and skip trying to be backwards-compatibility.
|
Pro: potential HD key support I know this is the source of my contention, and as a library implementor, users would love this BIP, but complain about no standard solution for BIP32 wallets. |
I think there is no reason users would need to deal with individual private key anymore. Though a standard for this for HD private and public keys would be useful. The way I am dealing with it now is by requesting users to specify the HD key + the first address of his wallet, and I try to know what type of derivation to use. |
I was importing private keys from one node into another, which is what made me interested in this in the first place, so I think this is still a useful feature. Electrum added their own suffixes for keys, and other wallet software may follow suite, so a BIP for this seems reasonable. Current wallet implementations (incomplete list):
|
Npm package If the alternative to this BIP is 100 years of bike-shedding over BIP32 structures, I'd rather not. This BIP is still useful in isolation, and it might at least set a standard for the compression/script template encoding which could be used for BIP32 structures, if desired. |
75dd9a1
to
0060780
Compare
I would support it on NBitcoin side. |
Nit: Use MediaWiki formatting for the Concept ACK from me, and keep the BIP focused on single-address keys. SLIP-0032 already proposes a solution for an upgrade to HD seeds. |
bip-0178.mediawiki
Outdated
|- | ||
|<code>0x12</code>||P2WPKH_P2SH||Yes||Segwit nested in BIP16 P2SH (<code>3...</code>) | ||
|- | ||
|<code>0x13</code>||P2SH|| — ||Non-Segwit BIP16 P2SH (<code>3...</code>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What script type does this refer to? The slip32 prefixes don't allow arbitrary redeem scripts (it would defeat the point IMO). If this is meant to be P2PKH_P2SH, please refer to it as that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@afk11 IMHO it should be P2SH_P2PKH, as better represents P2SH(P2PKH)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this would be used for arbitrary P2SH scripts with a related private key (e.g. multisig). Is there a reason to even have a P2PKH in P2SH variant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kallewoof given the lack of redeem script context, wouldn't this be implemented as an 'unsupported' flag by most implementations?
Or is half-way-there still better than nothing?
Otherwise we are assuming the context and it is no better than providing a standard WIF?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, if I make a custom multisig with someone else, and I export the private key, even if I don't know the redeem script itself, it may be useful to know what the key was used for. An implementation that doesn't support custom redeem scripts would probably keep it unimplemented, but that's a good thing, right? They can't spend the UTXOs anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, my point is that if you're using a custom script/address, you must have more information. There is simply no way you could hope to have everything in one string. And there is no need for that, I think.
In the general case, you will provide your wallet with a bunch of information to describe what outputs you mean, and which private keys are relevant for it. In a few exceptionally simple (but remarkably common) cases, you get to put everything in one string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, conclusively, do not include types that we won't be able to use without additional information anyway. I.e. remove P2SH, P2WSH, and P2WSH_P2SH, and keep the rest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And then use 0x01=P2PKH_COMPRESSED for 'custom stuff' exactly like is done today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated BIP, removing the 3 script-dependent types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kallewoof I think having script-hash types is fine once the flag tells you what the redeemscript is. I see no harm in having P2SH_P2WPKH, P2WSH_P2PKH, and if we support producing those scriptPubKeys. Script hash scripts have no degrees of freedom besides the script they commit to, so IMO supporting script-hash extensions is pretty natural.
We could leave it the way it was before, assuming they refer to P2PKH..
Or we could use bits 2 and 3 to indicate P2SH and P2WSH, and maybe use others to indicate the scriptPubKey type?
Another, is just ignore script-hash usage entirely, and assume the software will deal with it.
bip-0178.mediawiki
Outdated
|- | ||
|<code>0x14</code>||P2WSH|| — ||Native Segwit P2SH | ||
|- | ||
|<code>0x15</code>||P2WSH_P2SH|| — ||Native Segwit P2SH nested in BIP16 P2SH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above - both of these are script-hash, not script types to be run.
bip-0178.mediawiki
Outdated
|- | ||
|<code>0x13</code>||P2SH|| — ||Non-Segwit BIP16 P2SH (<code>3...</code>) | ||
|- | ||
|<code>0x14</code>||P2WSH|| — ||Native Segwit P2SH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above - is this P2WSH P2PKH?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK - in a similar vein to key compression, think things are a lot simpler if we know the script type from it's encoding.
I do think we should specify the redeemScript types in places where we just use 'P2SH' or 'P2WSH', if it's P2PKH we should be clear about it.
It does seem there's a dependency between the prefix spec for BIP32, and this BIP. I guess implementations should fail if someone attempts to use BIP32 a SLIP132 prefix for which there is no WIF prefix? If someone were to export a WIF from a SLIP132 key, they would need to be in sync. |
I'm not sure what you mean. |
|- | ||
|<code>0x01</code>||P2PKH_COMPRESSED||Yes||Compressed legacy public key. Unknown public key format | ||
|- | ||
|<code>0x10</code>||P2PKH||Yes||Compressed legacy public key. Legacy public key format (<code>1...</code>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the semantic difference between 0x01
and 0x10
here?
Unknown public key format
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dcousens 0x10 means it is a 1.. key, and nothing else. 0x01 means it can be any kind of key (i.e. the key was made before the BIP-178 was implemented so you don't know what kind).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kallewoof a 1...
key... so P2PKH?
Are you suggesting that this is for users who generated the uncompressed form from the compressed key given, and therefore the targeted compression [for the address] is ambiguous?
If so, ACK, but, that wasn't clear to me from the text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. The 0x01
is "old format, we don't know what kind it is". The 0x10
is "this is a legacy non-segwit p2pkh". I'll update to clarify.
With the limited scope limited only to N=1 public-key scripts, I do wonder if a different format (bech32 encoded maybe) wouldn't be simpler and safer for implementations compared to incompatibly changing the WIF format. |
@clarkmoody D'oh. Thanks, fixed! |
Hmm, any reason against including the script types exposed in slip132? It's possible people would be dumping WIF's for those keys, and lacking a defined script type for them (P2SH_P2WSH_P2PKH and P2WSH_P2PKH) people would just use \x01 I did see discussion about script-hash types, but it the discussion seemed to be against arbitrary redeem & witness script types, these would explicitly be P2PKH. |
@kallewoof sorry for the delay! thought I'd got back to this already. First I was sure @clarkmoody is it intended that It really would be nice, as then P2WPKH and P2PKH (in all script-hash cases) would have a prefix. |
@afk11 SLIP-0132 is mainly following the lead of Electrum in the x- y- z-pub version bytes. |
@clarkmoody OK, thanks for answering! (off topic, but it might be worth mentioning on slip132 that P2SH, P2WSH, P2SH|P2WSH refer to BIP11 scripts) @kallewoof - yep, all good. |
I am very confused by the usage of the term "public key" in this BIP. An ECDSA public key will always remain a public key, it can be uncompressed or compressed, but there is not different kinds. It seems to me that the term "bitcoin address" should be use instead.
We should say "There are several types of bitcoin addresses..." It's even more confusing when we are talking about pay to script hash. |
While a compressed public key and an uncompressed public key are different types of public keys, the same doesn't really go for the other kinds, so yeah, I overall agree with your point. I think "bitcoin addresses" makes more sense. Will fix. |
Thank you for the quick fix. I would like to express the same criticism regarding the term "Typed Private Keys". |
I'm not sure I agree. The format describes a private key, with a type, so "typed private key" isn't too far off, I think. Will think more about it. |
What about "Version bytes extended WIF"? |
It's not really a version, I think. "Type extended WIF"? "Address type extended WIF"? |
BIP32 defines the extended key serialization format with 4 bytes "version bytes" to encode the network (https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#serialization-format). |
That's a good point, yeah. "Version extended WIF"? 'bytes' seems unnecessary (and it's only one byte). "VEW" is a horrible abbreviation though. "XWIF"? Hum. |
Is this ready for merging? |
@luke-jr Yes, I believe so. |
"Version extended WIF" sounds good to me, but I don't know, it would be good to have more inputs. |
@kallewoof The abstract still says "...to specify what kind of public key the private key corresponds to.", instead of "bitcoin addresses". Sorry to bother you with that but it is an important detail. |
@Janaka-Steph maybe PR the changes you feel are necessary so they can be reviewed / ACKed if ok? the PR has already been merged :) |
I'm PR'ing the change now. Thanks for the pointer, @Janaka-Steph. |
No description provided.