Support ypub/zprv, plus custom addresses #1014

afk11 · 2018-03-04T19:49:56Z

This PR adds a new parameter to the hdnode constructor. The parameter describes a bip32 prefix, and has the public & private prefix values, plus a scriptFactory field. This exposes a function, convert(ECPair key) and produces:

{
  scriptPubKey: Buffer,
  signData: {
    redeemScript: x,
    witnessScript: y
  }
}

also updated fromSeedHex, so you can persistently mark a key and it's children with a prefix
updated fromBase58, if no list of networks is provided (in which case, we only compare against .bip32, we assume the network is an object (or take bitcoin as the default), and in this case will check the prefix from the key against the .private and .public members of the list of prefixes.
if no prefix is set, we default to p2pkh.
getAddress was modified to use the script factory, by doing address.fromOutputScript() on the resulting scriptPubKey - please see the test I commented out, and lets talk about that! https://github.com/bitcoinjs/bitcoinjs-lib/pull/1014/files#diff-4db2c2d53279dc2a5cf3fee520cbb23eR126
added the bip49 and bip84 prefixes to networks.
added tests, basically just covering the expected behavior of a key with a prefix, using the fixtures from slip132

Not sure what direction this are going with the new templates/script stuff, so all ears for feedback on the stuff in keytoscript!

afk11 · 2018-03-04T19:59:55Z

src/keytoscript.js

+var bcrypto = require('./crypto')
+var btemplates = require('./templates')
+
+function checkAllowedP2sh (keyFactory) {


these two functions suck, doesn't really allow people to bring their own functions.. but wanted run-time prevention against nesting script hash types incorrectly.

afk11 · 2018-03-04T20:00:07Z

src/keytoscript.js

+  }
+}
+
+function checkAllowedP2wsh (keyFactory) {


afk11 · 2018-03-04T20:01:24Z

test/hdnode.js

+      //     .once().withArgs().returns('foobar')
+      //
+      //   assert.strictEqual(hd.getAddress(), 'foobar')
+      // }))


Should discuss whether keys have an associated address type. Maybe following keytoscript, we should just assume some function is responsible for converting keys to their scripts, and then addresses, rather than having getAddress and trying to manage defaults.

…get hdnode.getAddress

afk11 · 2018-03-04T20:09:12Z

haven't tested all of the the key-to-script functions, will leave that after I get feedback on the approach being taken. the prefixes/script functions used in tests obviously work :)

dcousens · 2018-03-05T02:08:08Z

Did you look to https://github.com/bitcoinjs/bitcoinjs-playground which should land in 4.0.0 for how these might integrate?
There is definitely a lot of double up 😕

afk11 · 2018-03-05T10:53:55Z

Is it this PR needs refactoring first, or we need to wait until that gets merged? Been looking that as well alright after your note on the other PR. Maybe the closures can just pass it to something like

const { address } = bitcoin.payments.p2sh({
  redeem: bitcoin.payments.p2wpk({
    pubkey: pubKey
  })
})

It confused me a little though as it has a lot of emphasis on inputs, but yea, all I need is address/scriptPubKey/rs/ws!

dcousens · 2018-03-05T23:32:20Z

It confused me a little though as it has a lot of emphasis on inputs, but yea, all I need is address/scriptPubKey/rs/ws!

Can you elaborate on what you mean by this?

, or we need to wait until that gets merged?

I think it is this one, otherwise we'll end up with more cross-dependence that could make the merge of that PR difficult (or require swathing changes that might have been released here).

afk11 · 2018-03-06T11:19:39Z

It confused me a little though as it has a lot of emphasis on inputs, but yea, all I need is address/scriptPubKey/rs/ws!

Yea - with the emphasis on inputs, witnesses, and decoding all the information from the object, it looks less like it's meant to make the data I need. Signature parsing normally happens in a separate step to making the output script you'd request payment on..

Though, it means something like this might work:

hdnode.getScriptData -> (p2wsh(p2wpkh()) -> { output scripts }
hdnode.getScriptData(input) -> (p2wsh(p2wpkh()) -> { output scripts + input signatures, etc }

dcousens · 2018-03-06T14:33:25Z

That aside, I really don't think tagging keyPairs or HDNode with script information is the right outcome. It is unfortunate that ypub/zpub has done that, but, it really seems like 1 particular decision is trying to re-organize several code bases...
We wanted to remove getAddress, not encode it further...

afk11 · 2018-03-06T15:06:02Z

Yea I don't like it on key-pairs much either. Electrum also proposes we start doing script-types with WIF's, which I think is awful, but at least they're the only ones doing it for now :P

The hate for getAddress was because we had no control over the script type, and couldn't change it without consumers being affected. This PR would actually fix that problem for a hdnode, surely? If still desired we can remove it from ECPair once we have a method for getting a script from a key.

I've had requests for this feature elsewhere as well, since two wallets (electrum & trezor) have implemented it, so I don't see it going away..

So instead of scrapping it, we can try carve a useful feature out of it? As it stands, users generally have to plug primitives together (scripts/templates/crypto,etc) to get an address, so I think exposing redeem/witness/spk script generation, and thereby addresses, is a really rapid way to get people generating addresses and signing without worrying about a tonne of internals (like script types, whether it's p2sh, p2wsh, and the address type!)

dcousens · 2018-03-06T15:31:40Z

, so I think exposing redeem/witness/spk script generation, and thereby addresses, is a really rapid way to get people generating addresses and signing without worrying about a tonne of internals (like script types, whether it's p2sh, p2wsh, and the address type!)

Isn't that exactly what the bitcoinjs-playground code provides? A safe and complete abstraction for users to replace the clunky-ness of tag-teaming the right incantation for payment type?

So instead of scrapping it, we can try carve a useful feature out of it?

Agreed! But what feature is that to be? :)

afk11 · 2018-04-03T11:28:22Z

In this case, I see the useful feature just being being able to produce scriptPubKeys/redeemScripts/witnessScripts/addresses for the HD key.

Instead of shooting straight for pubkey -> address, we can go pubkey -> scripts -> address, and also make it a useful way to get the scripts for signing. And, I realize before that getAddress seemed it was only convenient for P2PKH people, but we didn't have any situations where prefixes indicate a script type

dcousens · 2018-04-12T04:10:45Z

See BIP178, this is probably what we will do.

dcousens · 2018-04-12T04:12:04Z

@afk11 maybe we should hit the discussion up with how this will be handled with extended keys?

dcousens · 2018-07-19T09:34:17Z

@afk11 what are you're thoughts on this now with the 4.0.0 API?

afk11 changed the title ~~allow custom bip32 prefixes, and varying types of addresses returned …~~ Support ypub/zprv, plus custom addresses Mar 4, 2018

afk11 commented Mar 4, 2018

View reviewed changes

src/keytoscript.js

}

}

function checkAllowedP2wsh (keyFactory) {

Copy link

Contributor Author

afk11 Mar 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

afk11 commented Mar 4, 2018

View reviewed changes

allow custom bip32 prefixes, and varying types of addresses returned …

f44e662

…get hdnode.getAddress

afk11 force-pushed the hk-custom-address branch from 54eb425 to f44e662 Compare March 4, 2018 20:04

afk11 mentioned this pull request Mar 4, 2018

Support for BIP49 YPUB/UPUB #927

Closed

hdnode: expose getScriptData

84e330d

dcousens self-requested a review March 5, 2018 02:06

dcousens self-assigned this Mar 21, 2018

dcousens added the feature label Mar 21, 2018

dcousens closed this Jul 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ypub/zprv, plus custom addresses #1014

Support ypub/zprv, plus custom addresses #1014

afk11 commented Mar 4, 2018 •

edited

afk11 Mar 4, 2018

afk11 Mar 4, 2018

afk11 Mar 4, 2018

afk11 commented Mar 4, 2018

dcousens commented Mar 5, 2018

afk11 commented Mar 5, 2018 •

edited

dcousens commented Mar 5, 2018 •

edited

afk11 commented Mar 6, 2018 •

edited

dcousens commented Mar 6, 2018

afk11 commented Mar 6, 2018 •

edited

dcousens commented Mar 6, 2018 •

edited

afk11 commented Apr 3, 2018 •

edited

dcousens commented Apr 12, 2018

dcousens commented Apr 12, 2018

dcousens commented Jul 19, 2018

Support ypub/zprv, plus custom addresses #1014

Support ypub/zprv, plus custom addresses #1014

Conversation

afk11 commented Mar 4, 2018 • edited

afk11 Mar 4, 2018

Choose a reason for hiding this comment

afk11 Mar 4, 2018

Choose a reason for hiding this comment

afk11 Mar 4, 2018

Choose a reason for hiding this comment

afk11 commented Mar 4, 2018

dcousens commented Mar 5, 2018

afk11 commented Mar 5, 2018 • edited

dcousens commented Mar 5, 2018 • edited

afk11 commented Mar 6, 2018 • edited

dcousens commented Mar 6, 2018

afk11 commented Mar 6, 2018 • edited

dcousens commented Mar 6, 2018 • edited

afk11 commented Apr 3, 2018 • edited

dcousens commented Apr 12, 2018

dcousens commented Apr 12, 2018

dcousens commented Jul 19, 2018

afk11 commented Mar 4, 2018 •

edited

afk11 commented Mar 5, 2018 •

edited

dcousens commented Mar 5, 2018 •

edited

afk11 commented Mar 6, 2018 •

edited

afk11 commented Mar 6, 2018 •

edited

dcousens commented Mar 6, 2018 •

edited

afk11 commented Apr 3, 2018 •

edited