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

Fix PrivateKey.toBuffer #48

Closed
wants to merge 1 commit into from

Conversation

fanatid
Copy link
Contributor

@fanatid fanatid commented Feb 25, 2016

Issue: #47

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

braydonf commented Mar 7, 2016

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
Copy link
Contributor Author

fanatid commented Mar 7, 2016

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
Copy link
Contributor

afk11 commented Jul 5, 2016

@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
Copy link
Contributor

braydonf commented Jul 5, 2016

@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

@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
Copy link
Contributor

afk11 commented Jul 5, 2016

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

@braydonf
Copy link
Contributor

braydonf commented Sep 12, 2016

@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
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
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