Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removal of JSBN #117

Merged
merged 10 commits into from Apr 21, 2014
Merged

Removal of JSBN #117

merged 10 commits into from Apr 21, 2014

Conversation

dcousens
Copy link
Contributor

@dcousens dcousens commented Apr 6, 2014

In this pull request JSBN has been replaced with the module bigi.

An extra method BigInteger.fromBuffer has been patched in through the use of src/bigi.js.
This provides the utility functionality for Buffer -> byte array conversions currently required for several parts of the library.
This is undoubtedly not the ideal solution for this, however provided we can merge a fix for upstream to support Buffers, this is no less 'hacky' than the previous amendment that existed.

Although this isn't actually a replacement (#24), as bigi is actually just JSBN anyway; it does externalize the testing and maintenance for this module, and therefore place less responsibility on this library to maintain the dependency independently.

This pull request should (probably?) not be merged until the the tests that previously existed are merged up stream.

The test coverage of JSBN has always been poor, and this pull request does not change that (however it should make it easier in the long run).

edit: as in #24 , this may be a good opportunity for a better big number library.

@kyledrake
Copy link
Contributor

I'm +1ing this. @weilu can you give a +1 here, any issues?

@weilu
Copy link
Contributor

weilu commented Apr 17, 2014

Once the ec code has been cleaned and refactored, this needs to be rebased off that. I have this glimpse of hope that the ecdsa implementation then might be independent of the internal bit representation of big integer, which will allow us to move to native big integer modules like bigint or bignum for better performance on node.

@caedesvvv
Copy link
Contributor

They say: "Please don't use this library for actual big integer operations. We may aggressively prune the library so that it just supports operations necessary for CryptoCoinJS."

Not sure how safe it is for bitcoinjs, doesn't sound safe for apps also using BigIntegers for more things or crypto outside of cryptocoinjs.

@caedesvvv
Copy link
Contributor

...and want to keep one set of implementations inside for each primitive

@kyledrake
Copy link
Contributor

@caedesvvv let's ask them. @jprichardson thoughts on the above comments?

@jprichardson
Copy link
Member

BitcoinJS and CryptoCoinJS use a Big Integer library for the exact same thing. What we were trying to communicate was that it's probably not safe for someone to develop a math application or something of that nature on top of bigi as the purpose of bigi is crypto currency applications. Anything outside of crypto currency applications would be out of scope for us.

It may be beneficial to prune it though so that payload is smaller in the browser. At any rate, we don't have any formal plans to do this soon and are more than willing to work with the devs of BitcoinJS to make sure that applications aren't broken. But given that our use case is identical, I don't foresee any problems.

Moving forward, we're looking for a Big Integer library that is Buffer based. @sidazhang, @MidnightLightning and I have talked about writing one but haven't committed to anything yet.

Hopefully this clears this up. Happy to address any concerns.

@caedesvvv
Copy link
Contributor

Well, first I see crypto as a (very tricky and precise) Math application. That's why I was worried about the comment where it says explicitly about not using it and dark plans for the future :-P.

I think a good biginteger library is a good payload to support in a browser application. If it only works for certain purposes then it would cause problems integrating other libraries, like I had to hook the bitcoin BigInteger into curve25519 code so i wouldnt need its jsbn dependency... just works (for obvious reasons), other codes seem to use the same api.

Final application developers are free to prune, well ok as long as everyone can choose but I fear a future where every lib just assumes their custom version and will end (as final application developer) ending with milliard dependencies included under the hood.

@dcousens
Copy link
Contributor Author

Rebased on #145.

Can we please get a final +1 for this?
This doesn't mean we are stuck with using bigi, in fact I think its assumed we are going to change it in the near future.
But for now, this provides us with the necessary changes to be able to have that flexibility.

bigi is a good stepping stone due its 1:1 compatibility (it is literally JSBN), and means we can hopefully now begin to move our API's to bigint or bignum.

@kyledrake
Copy link
Contributor

+1

kyledrake added a commit that referenced this pull request Apr 21, 2014
@kyledrake kyledrake merged commit 4689abb into bitcoinjs:master Apr 21, 2014
@dcousens dcousens deleted the rmjsbn branch April 21, 2014 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants