ecdsa.recoverPubKey takes a "curve" parameter that is private

[glorat]

ecdsa.recoverPubKey takes "curve" as its first parameter. In all likelyhood, what needs passing in here is

var curve = ecurve.getCurveByName('secp256k1')

so the parameter is arguably redundant. However, having been forced to pass this curve, I can't find a way to actually do so! That's because both ecurve and ecurve.getCurveByName are private to the library as far as I can tell and the test case in ecdsa.scala cheats by requiring curve.

In practice, I can of course require("ecurve") in my library myself but this falls foul of the "instanceof" trap as described at http://www.mattesch.info/blog/the-instanceof-trap-in-node-js

Passing in my own curve does make the key recovery work but I then cannot pass the Q into ECPubKey constructor due to the instanceOf check which I have no way of passing.

I hope this makes sense... Possible fixes include:

• Remove all instanceOf checks in the constructors of things like ECPubKey
• Make ecurve or ecurve.getCurveByName exported from the bitcoin module
• Remove curve as a parameter from recoverPubKey, or make it optional defaulted to the bitcoin cure
  I vote for the latter!

[weilu]

I had a lengthy discussion with @dcousens when he was refactoring ecdsa.js. I was of the opinion that curve arg should be omitted, as bitcoin alongside a few other networks we support at the moment(doge and lite) all use the same curve. Until we have the need to support specifying another other curve or extracting ecdsa as a separate module, without the curve param makes the API cleaner.

my vote goes to option 3. And I prefer removing the curve arg over keeping it as an optional arg. It's up to @dcousens

[glorat]

In the mean time, I've created a gist based on the Brainwallet code against bitcoinjs-lib 1.0.3. (This is an "upgrade" of the Brainwallet code that is still on 0.1.3). All licenses are pretty open here so feel free to adapt this into your "integration" tests.
https://gist.github.com/glorat/2179505f380b9984cb4f

[Shic1983]

Hey @glorat ~ It puts a smile on my face to see you wanting to do this because literally about 12 weeks ago I was wanting to do EXACTLY this same thing. I was commenting to @dcousens in IRC (#bitcoinjs-dev on Freenode) that this functionality was there in 0.1.3 but now not :( . . .In the end I actually gave up on it.

[dcousens]

@weilu option 3 simply side steps the issue that we have external dependencies present in our API, and when we use instanceof to enforce that, it will break when two versions are used.

This problem will surface identically with HDNode, ecdsa and ECKey in relation to the BigInteger's in their API.

The only consistent solution I can see thus far is that we remove the instanceof checks, OR expose those dependencies. That is, options 1 and 2.

[dcousens]

@glorat the hate-yourself solution right now (until this is resolved) is:

BigInteger = ECKey.makeRandom().d.constructor, secp256k1 = ECKey.makeRandom().pub.Q.curve

[weilu]

Well, I guess there are two issues here, the simplest, not hate-yourself solution that solves @glorat's current issue is to remove the curve arg in 2.x.x

I agree that the reported issue does expose the instanceof checks on objects that are from our dependencies. A quick git grep of instanceof shows that we only have 4 instanceof checks on BigInteger and 1 on ecurve.Point. We could PR bigi and ecurve to include isBigInteger and isPoint methods and use them instead of instanceof. Lucky that both libraries are free of dependencies. So we should be good for the case of same versions of the dependencies.

If an app includes an earlier version of bigi or ecurve than what we have, calls to isBigInteger and isPoint will just throw an error because of method not defined. It certainly isn't the most friendly error, but it safely stops the execution. If we want to make it more friendly, we could catch the error and suggests that they upgrade the corresponding dependency if it is included in their app as a direct dependency. But I guess it would be more of an edge case.

[weilu]

Per discussion in IRC, the above described interface checking methods can't guarantee safety. We could end up with cases where two different versions of the same library both yield true for isBigInteger while their underlying implementations differ drastically, which could lead to unfortunate errors in calculations.

At this point, exposing our dependencies seems like the simplest and least harmful solution to the instanceof problem.

If only npm only allows a single version of a library (yes arguments for dependency hell), our lives would be so much easier.

[glorat]

Fortunately I don't need to be hateful as I am quite happy commenting out the isInstanceOf checks in my local to get by until people decide on the best way.

If anything, I'm glad you guys are thinking hard about this. Exposing your dependencies is a step with grave consequences - as is allowing the injection of any old library.

[dcousens]

Regardless of what happens with #231, I think option 3 is a suitable step until we move ECDSA out of this library entirely.