search

dashpay / electrum-dash-old

Electrum-DASH - a Dash thin client
https://electrum-dash.org
MIT License
37 stars 113 forks source link

use Dash BIP32 keychain values #7

Closed nmarley closed 8 years ago

nmarley commented 8 years ago

Dash BIP32 xpub/xprv keys should begin with "drkv"/"drkp".

Kefkius commented 8 years ago

Honestly, I don't see the point in chain-specific base58 versions for extended keys, particularly in this context. Many other tools expect xpub/xprv, so you may end up with less convenience if you want to use your electrum-dash keys elsewhere.

That being said, I'm willing to change the unit tests to use these new prefixes if people prefer that we merge this PR.

nmarley commented 8 years ago

I've been thinking about this too. The only downsides are conversion of already-generated Dash BIP32 extended keys (which can be re-generated via seed).

On the other hand, there are other coins which exist, and I can tell from a glance which key chain this is for. If we leave xpub/xpriv, there's the ambiguity. But all that aside, a standard has already been established in the direction of currency-specific and is followed in the reference client since 2014, specifically here:

https://github.com/dashpay/dash/blob/master/src/chainparams.cpp#L172-L173

Also, other projects which use this will also expect and use the drkp/drkv keys, because it's in the reference code and it's been established by convention.

Here's another example:

https://github.com/UdjinM6/bitcoinjs-lib-dash/blob/master/src/networks.js#L52-L53

In my opinion, it is the right thing to do because of interoperability. The keychains should be the same in all software so that it works together & doesn't cause lock-in or problems when users want to migrate or when other software solutions are integrated.

Kefkius commented 8 years ago

BTW it seems the values for BITCOIN_HEADER_PRIV and BITCOIN_HEADER_PUB are messed up. BITCOIN_HEADER_PRIV should be 02fe52f8 and BITCOIN_HEADER_PUB should be 02fe52cc.

nmarley commented 8 years ago

Nope, not according to the reference client:

https://github.com/dashpay/dash/blob/master/src/chainparams.cpp#L172-L173

See the discussion on Dashtalk for more info on that:

https://dashtalk.org/threads/dash-bip32-serialization-values-dev-discussion-wont-apply-to-most.8092/

On Sun, Mar 6, 2016 at 12:51 AM, Kefkius notifications@github.com wrote:

BTW it seems the values for BITCOIN_HEADER_PRIV and BITCOIN_HEADER_PUB are messed up. BITCOIN_HEADER_PRIV should be 02fe52f8 and BITCOIN_HEADER_PUB should be 02fe52cc.

— Reply to this email directly or view it on GitHub https://github.com/dashpay/electrum-dash/pull/7#issuecomment-192698076.

Kefkius commented 8 years ago

Bleh, that means my PR is wrong. We should change it anyway to something more DASH-y.

nmarley commented 8 years ago

I moved this to a different branch, so it auto-closed.

This would cause problems unless a shim were written to convert both xpub values and the xprv values, which could only be done upon wallet unlock.

Feel free to re-open it, but given all the library discrepancies which already exist, I think that in the future most devs will probably have to deserialize the xkeys and look at the private/public bits anyway. So I really don't care about the xpub values anymore.