Adding path derivation #538

karelbilek · 2016-02-06T01:08:04Z

Now this might be a bit controversial, but I found something like that to be really useful.

What will be controversial, I think:

the usage of strings as indexes
- yes, it's not ideal, but adding an apostrophe is a way to easily say "this is hardened" and this notation is common, and you cannot do that otherwise
the usage of descriptions as paths in the tests
- again, not ideal, but I didn't want to repeat the description as path again if it's already there

dcousens · 2016-02-06T02:29:48Z

ping @jprichardson, merge here, then extract to BIP32?

dcousens · 2016-02-06T02:30:11Z

src/hdnode.js

@@ -281,6 +281,24 @@ HDNode.prototype.deriveHardened = function (index) {
  return this.derive(index + HDNode.HIGHEST_BIT)
 }

+// number + ' (e.g. 234') as a string -> hardened
+HDNode.prototype.derivePath = function (path) {


Any reason why this just doesn't take a string as input?

dcousens · 2016-02-06T02:32:01Z

IMHO, I think node.derivePath("m/0'/1/0") would be a better API, aka a String argument.

I don't think there is much value in adding an Array based path API. You can trivially do that using derive and forEach already.

dcousens · 2016-02-06T02:33:02Z

We also intend to break out BIP32 to its own module, probably encompassing HDNode standalone with other utilities for making life easier when dealing with them.

See #508 (comment)

I'm not sure if adding these API's flies in the face of that or not.

karelbilek · 2016-02-06T02:48:57Z

ad array based API:

My original thinking for using Array was "What if someone puts there an array that is not a path, or that is wrongly formatted? Do I have to trim spaces around the slashes? Does it have to start with 'm/' or not? What if the node is not a master and is a sub-node, the 'm/' doesn't make sense", and that by using Array I just go around all these problems.

Then I also thought about the function accepting both array and string, but that would be really ugly.

ad splitting:
sure, great idea.

jprichardson · 2016-02-06T15:06:17Z

ping @jprichardson, merge here, then extract to BIP32?

@dcousens Yep, I don't see any harm in adding it at the moment. Agreed on not needing the array API.

karelbilek · 2016-02-06T15:47:06Z

how exactly would you do the string-based API? Should it start with 'm/', or should it be "relative"? So for example,master.derivePath("0'/1/0'/1")(which would equal to master.derivePath("0'/1").derivePath("0'/1"))

dcousens · 2016-02-06T16:36:31Z

I think 0'/1/0 is a suitable enough API.
We could easily regex, optionally skipping any prefixes until the first number is reached?

karelbilek · 2016-02-08T12:46:10Z

By "we", you mean the code in the library, or the user of the API?

dcousens · 2016-02-08T17:10:47Z

We, the developers of this library.
My suggestion was that we could make the m/ optional, but, ambiguous API's suck, so, not sure.

karelbilek · 2016-02-08T21:17:52Z

ambiguous API's suck

Exactly. But then, I don't know how to make it unambiguous and non-sucky.

And I don't also want to totally abandon it either. 😓

dcousens · 2016-02-09T02:54:26Z

@Runn1ng I think if an m/ is present, we check that !this.parentFingerprint (aka, this is a master node).
If it isn't present, proceed on with derivation.

Thoughts?

karelbilek · 2016-02-11T14:55:06Z

Sorry for the delay. OK, that sounds reasonable

karelbilek · 2016-02-12T02:28:19Z

Done :)

dcousens · 2016-02-12T03:02:05Z

src/hdnode.js

+      splitPath = splitPath.slice(1)
+    } else {
+      throw new Error('Not a master node')
+    }


if (this.parentFingerprint) throw new Error('Not a master node') splitPath = splitPath.slice(1)

I am not sure standard allow if block without curly braces :) but yeah I got you. Ok, will do tomorrow

The argument is path of either numbers or strings. String with "'" at the end signifies hardened path.

I am taking description field for the path.

Maybe overdoing it a bit :)

karelbilek · 2016-02-12T12:01:59Z

I have done some cosmetic changes and rebased. The failing check has nothing to do with my code.

dcousens · 2016-02-13T00:01:50Z

Sounds good, will review asap :)

fanatid · 2016-02-13T08:12:56Z

@Runn1ng why unary plus instead praseInt?
with unary plus string / equals 0/0 because +'' gives 0, with parseInt string / throw error.

return splitPath.reduce(function (prevHd, indexStr) {
  if (indexStr.slice(-1) !== "'") return prevHd.derive(parseInt(indexStr, 10))
  return prevHd.deriveHardened(parseInt(indexStr.slice(0, -1), 10))
}, this)

karelbilek · 2016-02-13T14:46:17Z

Good catch.

+ can cause issues - +"" is 0. parseInt("", 10) is NaN, which is better (since it causes typeforce to throw).

karelbilek · 2016-02-13T14:53:30Z

Just thinking.... if you put in path with decimal points ("m/0.5/1") by some mistake, using "parseInt" will silently truncate it instead of throwing errors.

But I am not sure that is worth caring about.

fanatid · 2016-02-13T15:07:53Z

As option, you can check that result of parseInt is equal original string, something llike this:

return splitPath.reduce(function (prevHd, indexStr) {
  var derive = prevHd.derive
  if (indexStr.slice(-1) === "'") {
    derive = prevHd.deriveHardened
    indexStr = indexStr.slice(0, -1)
  }

  var index = parseInt(indexStr, 10)
  if (index.toString() !== indexStr) throw new Error(...)

  return derive.call(prevHd, index)
}, this)

karelbilek · 2016-02-13T15:11:42Z

I am also thinking if things like m/ 2 / 0' / should be allowed. parseInt will munch it just fine. (parseInt(" 3 ", 10) === 3)

I will probably do what you suggest.

Alternatively, I can just test the index with regexp if it's only numbers.

dcousens · 2016-02-13T15:16:05Z

/([m]\/)?([0-9]+[']?\/)*([0-9]+[']?)/

karelbilek · 2016-02-13T15:39:58Z

Well, I wanted to test indices one by one after splitting, which is more readable and easily debuggable, but it's true that testing the parameter at the start of a function is more in line with the rest of the code.

Again through typeforce

karelbilek · 2016-02-13T22:27:12Z

I have added path validation to typeforce types.

dcousens · 2016-02-14T10:28:14Z

src/types.js

@@ -26,6 +26,11 @@ function UInt53 (value) {
    Math.floor(value) === value
 }

+function Path (value) {


BIP32Path? shrug

It's private anyway, so I'm not worried too much.

+1 for BIP32Path or BIP32DerivePath

Only thing I haven't been able to do is to add "undefined" to fixtures. So I could not properly test that node.derivePath() properly fails as it should. However, I added "null" there, and "null" and "undefined" behave in similar way in JavaScript, so that should catch that.

karelbilek · 2016-02-14T15:15:47Z

OK, feedback was put in

fanatid · 2016-02-14T15:34:14Z

ACK

dcousens · 2016-02-14T23:16:24Z

ACK 👍

Adding path derivation

karelbilek · 2016-02-14T23:25:36Z

😎 great

dcousens · 2016-02-15T01:57:57Z

test/fixtures/hdnode.json

@@ -109,7 +109,7 @@
          "address": "19EuDJdgfRkwCmRzbzVBHZWQG9QNWhftbZ"
        },
        {
-          "description": "m/0/2147483647",
+          "description": "m/0/2147483647'",


I might rename this to path

That would be great

karelbilek · 2016-08-08T09:11:54Z

Thanks for merging, I kind of forgot about this PR :)

dcousens reviewed Feb 6, 2016
View reviewed changes

dcousens added the feature label Feb 6, 2016

dcousens added this to the 2.3.0 milestone Feb 6, 2016

dcousens self-assigned this Feb 6, 2016

dcousens reviewed Feb 12, 2016
View reviewed changes

karelbilek added 3 commits February 12, 2016 12:37

Adding path derivation

7defe6f

The argument is path of either numbers or strings. String with "'" at the end signifies hardened path.

Testing path derivation

7a61526

I am taking description field for the path.

Doubling down on the path testing

755eac5

Maybe overdoing it a bit :)

Using parseInt instead of + in path parsing

182698f

+ can cause issues - +"" is 0. parseInt("", 10) is NaN, which is better (since it causes typeforce to throw).

fanatid mentioned this pull request Feb 13, 2016

Bug in HDPrivateKey._getDerivationIndexes bitpay/bitcore-lib#46

Open

Validating path

d2b43f1

Again through typeforce

dcousens reviewed Feb 14, 2016
View reviewed changes

Simplifying path regex

3c56e80

dcousens added a commit that referenced this pull request Feb 14, 2016

Merge pull request #538 from runn1ng/patch-2

1704155

Adding path derivation

dcousens merged commit 1704155 into bitcoinjs:master Feb 14, 2016

dcousens reviewed Feb 15, 2016
View reviewed changes

Ashtonulliac mentioned this pull request Aug 1, 2019

Send my bitcoin $to ulliactrevor267@gmail.com Ashtonulliac/bitcoinjs-lib#1

Merged

Adding path derivation #538

Adding path derivation #538

Conversation

karelbilek commented Feb 6, 2016

dcousens commented Feb 6, 2016

Choose a reason for hiding this comment

dcousens commented Feb 6, 2016

dcousens commented Feb 6, 2016

karelbilek commented Feb 6, 2016

jprichardson commented Feb 6, 2016

karelbilek commented Feb 6, 2016

dcousens commented Feb 6, 2016

karelbilek commented Feb 8, 2016

dcousens commented Feb 8, 2016

karelbilek commented Feb 8, 2016

dcousens commented Feb 9, 2016

karelbilek commented Feb 11, 2016

karelbilek commented Feb 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karelbilek commented Feb 12, 2016

dcousens commented Feb 13, 2016

fanatid commented Feb 13, 2016

karelbilek commented Feb 13, 2016

karelbilek commented Feb 13, 2016

fanatid commented Feb 13, 2016

karelbilek commented Feb 13, 2016

dcousens commented Feb 13, 2016

karelbilek commented Feb 13, 2016

karelbilek commented Feb 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karelbilek commented Feb 14, 2016

fanatid commented Feb 14, 2016

dcousens commented Feb 14, 2016

karelbilek commented Feb 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karelbilek commented Aug 8, 2016