Skip to content

Conversation

@dcousens
Copy link
Contributor

This pull request migrates the ECKey API to be stricter and force the user to be explicit about their intent instead of detecting their intent from the parameters.

This is done through the following three new functions:

  • ECKey.fromBuffer,
  • ECKey.fromWIF, and
  • ECKey.makeRandom

The default constructor ECKey() accepts a BigInteger and compression flag, and is what the other 3 constructors use to instantiate an ECKey.

A point of discussion is the change in behaviour by the constructor when dealing with keys outside of the secp256k1 keyspace.
Previously, any key imported into ECKey would be automatically constrained/wrapped:

input.mod(ecparams.getN()) 

This pull request removes that implicit behaviour and instead asserts that the input is already constrained to be within the key space.

assert(D.compareTo(ecparams.getN()) < 0) 

This removes an unnecessary mod operation, while also stopping confusion from users who might think that 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF is a valid private key.
This behaviour still exists in ECKey.makeRandom as it must constrain the randomly generated key to within these bounds.

Although not necessary, I have maintained toHex/fromHex convenience wrappers, these exist until we can simplify down the test fixtures enough that there will not be a huge amount of new Buffer(..., 'hex') and toBuffer().toString('hex') modifications just to maintain compatibility.

This pull request also removes the incorrect behaviour of ECKey.toBytes() which would append a 0x01 compression flag to the raw private key.
This could lead to invalid private keys being imported when moving around between APIs (any sane implementation would fail, but you'd be surprised).
The 0x01 compression flag is a feature of the Wallet Import Format (WIF) only, and has been isolated as such.
This is why some of the expectedPrivateKey's in test/hdwallet.js have been altered (0x01 removed).

Conforms with #106 and partially resolves #113.

src/eckey.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

The name makes it pretty clear it needs a buffer. Is this assert really necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A good point, some of these have been artifacts from the migration/testing process.
Though, some of these also produce really misleading errors if the wrong type makes it through.

@kyledrake
Copy link
Contributor

I'm +1. @weilu you're the merge lead on this.

@kyledrake
Copy link
Contributor

Also, this is really excellent.

src/eckey.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we make 32 a constant?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, an alternative:

var pad = new Buffer(32 - buffer.length) pad.fill(0) return Buffer.concat([pad, buffer])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know the copy is much faster in most cases (1 less call to malloc), though I don't know whether it makes a difference here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess copy as opposed to concat saves one instantiation of buffer. Instantiation according to the benchmark is slow.

@dcousens
Copy link
Contributor Author

Fixed up the error handling, rebased on HEAD and removed some unnecessary asserts.
The removal of the asserts in the export functions imply that the user should not change any of the internal data of ECKey manually, doing so could mean that the output is wrong (a perfectly reasonable result).

@weilu
Copy link
Contributor

weilu commented Apr 18, 2014

Looks good to me.

I'm bothered by the fact that verify and getAddress are not accessible directly from an instance of ECKey prototype. I understand the reasoning behind it. But when I have a private key, I have to call key.pub.verify() and key.pub.getAddress() bothers me. Smells like violation of Uncle Bob's Law of Demeter. Shall we alias those two on ECKey's prototype?

weilu added a commit that referenced this pull request Apr 18, 2014
Migrates ECKey to stricter API
@weilu weilu merged commit 06cd6a6 into bitcoinjs:master Apr 18, 2014
@dcousens dcousens deleted the eckeystrict branch April 20, 2014 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants