search

bitcoinjs / bitcoinjs-lib

A javascript Bitcoin library for node.js and browsers.
MIT License
5.68k stars 2.1k forks source link

SegWit: HDNode support for BIP49 (P2SH segwit) #779

Closed karelbilek closed 7 years ago

karelbilek commented 7 years ago

BIP49 adds support for P2SH address verification (for segwit addresses).

Now I am not sure if you want to add this to HDNode now - if I understood it correctly, you want to move HDNode out of bitcoinjs-lib entirely. But if HDNode supports getAddress, it should probably also support something like getP2shSegwitAddress (well that name is stupid :) but you have the idea)

dcousens commented 7 years ago

Excellent question!

getAddress has until now had the implicit meaning of getP2PKHAddress... but with SegWit, we really want to encourage entirely new defaults. Rather than confuse everything, maybe getAddress should simply be removed.

afk11 commented 7 years ago

On supporting stuff like 44/45/49: Maybe a BIP49 class constructed with (purpose', coin_type', account', change, address_index).. Then we getAddress and getScriptPubKey on that class would return the right type of script, and address? Offering an API for this might avoid confusion over which address function to call, the wrapper would know.

Generally I'd suggest keeping support for fixed path BIP32 indexes in a library far away from HDNode - a recent wallet meetup in berlin saw >50% people say they didn't like BIP44/45/49, that they harmed interoperability, and didn't give any kind of guarantee for recovery, etc. You could use m/0' for everything with the same effect.

But yea, might be time to rename ECPair.getAddress to getP2PKHAddress, and finally add getP2WPKHAddress once the segwit soft-fork activates?

dcousens commented 7 years ago

@afk11 100% agree. I actually hadn't read https://github.com/bitcoin/bips/blob/master/bip-0049.mediawiki , and didn't know it was pertaining purely to the BIP44 derivation schema; in hindsight it makes sense why @runn1ng's question was centered around HDNode...

In any case, it did bring up the relevance of getAddress in general; for which the verdict is still out.

might be time to rename ECPair.getAddress to getP2PKHAddress, and finally add getP2WPKHAddress once the segwit soft-fork activates?

Maybe, but, what an ugly name; and it doesn't sound clean for adding new address types... maybe we can just find a better way to leverage the bitcoin.addresses namespace?

afk11 commented 7 years ago

Yea agree wrt the naming.. what about a set of functions which accept the addresses hash (validating the buffers size), but also functions which perform the hashing (asserts the right instance type; does hashing for user) as convenience functions?

dcousens commented 7 years ago

@afk11 definitely prefer the idea. Would also prefer those functions to return null instead of throwing all the time...