Skip to content
This repository was archived by the owner on Apr 3, 2019. It is now read-only.

Fix PrivateKey.toBuffer#48

Closed
fanatid wants to merge 1 commit into
bitpay:masterfrom
fanatid:fix/privatekey-tobuffer
Closed

Fix PrivateKey.toBuffer#48
fanatid wants to merge 1 commit into
bitpay:masterfrom
fanatid:fix/privatekey-tobuffer

Conversation

@fanatid

@fanatid fanatid commented Feb 25, 2016

Copy link
Copy Markdown
Contributor

Issue: #47

@fanatid fanatid force-pushed the fix/privatekey-tobuffer branch from 8c8a871 to 3ab3664 Compare February 25, 2016 16:07
@braydonf

braydonf commented Mar 7, 2016

Copy link
Copy Markdown
Contributor

Thanks for taking a look at this!

This has come up before (bitpay/bitcore#1270), and the solution was to use:

privateKey.bn.toString(16, 32); // base 16, zero padded to 32 bytes

The solution in this PR is also useful:

privateKey.bn.toBuffer({ size: 32 });

There may be implementations that don't make a distinction and would break as a result? So it could be good to keep this as optional for the time being, and put into a next major release watch list.

@fanatid

fanatid commented Mar 7, 2016

Copy link
Copy Markdown
Contributor Author

Agree, it should be major release, but major should be released ASAP.
Many of this bugs can be avoided if bitcore-lib will use cryptocoinjs/secp256k1-node (related #41)

@afk11

afk11 commented Jul 5, 2016

Copy link
Copy Markdown
Contributor

@fanatid secp256k1-node doesn't work in a browser, so unfortunately an alternative is still required.

@braydonf Padding shouldn't be optional behavior, you should get used to doing it anywhere you serialize to binary/hex, lest something like the following happens:

  1. Public key is compressed, and top 8 bits of Px are zero.
  2. Key is serialized to a length of 32 bytes instead of 33.
  3. Hash this, produce an address.
  4. Funds are unrecoverable

@braydonf

braydonf commented Jul 5, 2016

Copy link
Copy Markdown
Contributor

@afk11 FYI, it's not an issue with publicKey.toBuffer()

However there is an issue because of this: https://github.com/bitpay/bitcore-wallet-client/blob/1dba5651ed26e2f20e3fb33d9f66faec0152ce35/lib/common/utils.js#L74

re: @matiu

@jprichardson

Copy link
Copy Markdown

@fanatid secp256k1-node doesn't work in a browser, so unfortunately an alternative is still required.

@afk11 this is incorrect. From the readme:

Works on node version 0.10 or greater and in the Browser via browserify.

@afk11

afk11 commented Jul 5, 2016

Copy link
Copy Markdown
Contributor

@jprichardson woops, I assumed it was just the C bindings.

@braydonf

braydonf commented Sep 12, 2016

Copy link
Copy Markdown
Contributor

@fanatid I've created a 1.0.0 branch for collaboration on changes that need an API change. This issue affects hd key derivation, and will likely need this to be the default, but optionally disabled since there are already keys derived by hashing the non-zero padded value.

@braydonf

Copy link
Copy Markdown
Contributor

I've brought this into the 1.0.0 branch at: https://github.com/bitpay/bitcore-lib/tree/1.0.0

@braydonf braydonf closed this Sep 12, 2016
@braydonf

Copy link
Copy Markdown
Contributor

How this relates to hd derivation is handled in: #97

@fanatid fanatid deleted the fix/privatekey-tobuffer branch September 13, 2016 05:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants