haskoin / haskoin-core

Haskoin Core is a Bitcoin and Bitcoin Cash library
MIT License
522 stars 93 forks source link

BIP-44, BIP-49 + BIP-84 (and ypub/zpub encodings) support #393

Open mikelpr opened 4 years ago

mikelpr commented 4 years ago

I don't know whether to trust the library. some of these points possibly due to ignorance on how they're supposed to work and they may be alright, but others make me think something is actually wrong

  1. it won't import zpubs nor ypubs with xPubFromJSON/xPubFromEncoding
  2. it derives segwit addresses from xpubs - aren't they supposed to be derived only from ypub and zpub?
  3. I can't get matching derived witness addresses in haskoin when importing a BIP39 mnemonic in electrum as segwit and looking at the address tab (importing the xpub in haskoin via importing that same mnemonic as legacy in electrum and getting the xpub from there as I can't import the segwit one because haskoin won't import zpubs) (had more points but figured them out)

I know it might also be that electrum is doing things differently from everything else, but I think the zpub thing is standard by now

wraithm commented 4 years ago

Yeah, we should support BIP-49 (https://github.com/bitcoin/bips/blob/master/bip-0049.mediawiki) and BIP-84 (https://github.com/bitcoin/bips/blob/master/bip-0084.mediawiki). We currently do not, to my knowledge.

The segwit signatures work. This has the test suite for some basic signatures. It just doesn't use the standard encodings, ypubs and zpubs that you mention.

I think it wouldn't take much work to implement these encoding schemes.

wraithm commented 4 years ago

By the way, we don't really support BIP-44 in haskoin-core, so maybe I spoke imprecisely by saying we should support BIP-49/84. We should support those encodings though at least, or maybe consider implementing BIP-44/49/84 and various HD derivation schemes.

You can use xprvs with segwit to produce valid transactions. There's nothing necessarily invalid about that, maybe just non-standard relative to what other wallets produce. Haskoin-core isn't a full wallet. It's a set of common utilities that one might use to produce a wallet.

jprupp commented 4 years ago

I'd be completely fine merging a pull request for this feature, and releasing a new version of the library.

jdeblese commented 2 years ago

I've taken a stab at this, and have some prototype code that might be suitable. However, I've one question: how supportive should haskoin-core be of jumping borders between serialization formats and networks?

I.e., if a btcTest ypub is imported, should all derived keys automatically be btcTest ypubs, requiring an explicit extra step to change network and/or serialization format; or, should keys have no implicit network/format and only gain one at the time of export? I think the former makes sense, but the latter is closer to how haskoin-core currently operates.

jprupp commented 2 years ago

Ideally, if the address format itself encodes information about which network it is valid for, then the resulting internal data type decoded from said address should also do the same. So your intuition is correct: there should be an explicit extra step to convert between networks if the key isn't meant to be valid in that other network.

For example, when dealing with regular addresses, the library will only decode an address if it is valid in the network that is specified as an argument to the decoding function. Else the function will fail to do so. That's an acceptable means to comply with the safety requirements of the library.

jdeblese commented 2 years ago

I ended up entirely removing the check against the network when decoding an address, and am having the key store the version word unmodified. I've added a functions to test if a key is valid on a given network, as well as to migrate a key from one network to another. It passes tests, and I've also checked it against the BIP-84 test vectors.

jdeblese/haskoin-core/BIP-49-84

Storing the version word has an issue, however, in that deriving a public key from a private key requires some way to map the version word from its private value to its public value. Ideally that mapping would come from passing deriveXPubKey a Network instance, but that means all the functions that use deriveXPubKey (quite a number, including non-obvious ones like XPrvFP) need a new Network argument. Currently I've hard-coded it, but that's undesirable.

An alternative that I've been looking at would be to have the key store just the serialization format, which is translated into a version word in conjunction with a network. That would mean fewer changes to the existing API, and would simplify address generation. However, serialization becomes complex: (1) deserialize can't convert the version word to a format because it doesn't have access to a Network instance; and (2) putX_Key can fail because the key serialization format may not be supported by the given network. (1) can be fixed in the getXPrvKey wrapper, at the cost of that deserialize . serialize is no longer an identity, but (2) can only be resolved by writing out some default value, which may silently produce a key that is invalid on the requested network, very much a bad thing.

I'll continue to work on this, but would welcome any comments.

lontivero commented 2 years ago

I'll continue to work on this, but would welcome any comments.

I would suggest to forget about [yz](pub|prv) because the best structure/format to encode network+pubkey is xpub and other data like derivation path and script type can be expressed more conveniently by using output descriptors.

Another reason is that those encoding formats can be added later and almost effortless. In the Wasabi project we added limited support to export zpub for main and testnet with:

public static string ToZpub(this ExtPubKey extPubKey, Network network)
{
    var data = extPubKey.ToBytes();;
    var version = (network == Network.Main)
        ? new byte[] { (0x04), (0xB2), (0x47), (0x46) }
        : new byte[] { (0x04), (0x5F), (0x1C), (0xF6) };

    return Encoders.Base58Check.EncodeData(version.Concat(data).ToArray());
}
jdeblese commented 2 years ago

If the Key types don't contain a format or purpose field, there isn't really anything to be done for these BIPs, as the only thing they do is set a purpose field in the derivation path, and in the case of 49 and 84 set a script type. Functions can only be added to haskoin-core to do that automatically if the purpose is recorded in the key, but then the serialization should support it cleanly as well. If that's not book-kept in core, I don't think it's worth adding such functions since the calling software has to explicitly track purpose fields and script types anyway.

However, as you point out the first character of these keys can be changed arbitrarily, so is there actually any need to support BIP-44, 49, or 84 explicitly in core? The calling software could just everything to xpub and determines by itself the compliant derivation path and script type themselves. Core already provides all the necessary tools for that, and it's not particularly complex. We could even "support" this in core by having the Import functions automatically convert y, z to x and u, v to t, with the caveat that all exports will be x or t. edit: scratch that, it takes a bit more than that to convert the key properly. That conversion could be added to core...

(What output descriptors are you referring to?)

lontivero commented 2 years ago

That's precisely my point, i think there is no point on adding support for those formats.

What output descriptors are you referring to?

https://github.com/bitcoin/bips/blob/master/bip-0380.mediawiki

jdeblese commented 2 years ago

Ok, we're in agreement. I may still submit a pull request for a convenience function for changing the version word of a base58-encoded key, as I expect that to be a commonly-used operation.

Thanks for the link.