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

Expose libsecp256k1's sha256 function #86

Closed
wants to merge 5 commits into from
Closed

Conversation

chjj
Copy link
Contributor

@chjj chjj commented May 10, 2016

Quick and dirty. This would be really nice to have considering that it is roughly 2-4x faster than openssl (the double sha is especially fast since it doesn't require extra instantiation of js objects, unlike node's crypto module). I think it's reasonable to not expose an updateable object since the constant crossover between js->c++ adds overhead, and I can't think of that much in bitcoin that would require or benefit from it. Having it as a single function is just simple and fast.

Also updated the git submodule to point at the bitcoin-core org.

@wanderer
Copy link
Member

Looks good to me. @fanatid what do you think?

@fanatid
Copy link
Member

fanatid commented May 10, 2016

sorry, I don't think that is a good decision add sha256/hash256 to this package
in my opinion would better create package bitcoinjs/bitcoinjs-hash with sha256/ripemd160/hash256/hash160
@chjj can you move updating submodules to separate PR?

@dcousens
Copy link
Contributor

NACK

@@ -1,3 +1,3 @@
[submodule "secp256k1-src"]
path = src/secp256k1-src
url = https://github.com/bitcoin/secp256k1
url = https://github.com/bitcoin-core/secp256k1
Copy link
Contributor

Choose a reason for hiding this comment

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

ACK

@chjj
Copy link
Contributor Author

chjj commented May 11, 2016

@fanatid, I feel these do belong here rather than a separate hashing module. All the messages to be signed or verified in bitcoin are sha256 hashes. The way I see it, sha256 has a particular relevance to this library, moreso than ripemd160+sha256, etc.

@fanatid
Copy link
Member

fanatid commented May 11, 2016

@chjj one of the items why I do not want merge this, because bitcoin-core/secp256k1 has no interface for sha256 (I made same for ecdh only because bitcoin-core/secp256k1#352 is stuck)

@fanatid
Copy link
Member

fanatid commented May 13, 2016

commit with updating secp256k1 url cherry-picked

@fanatid
Copy link
Member

fanatid commented May 13, 2016

other -- sorry, maybe in bitcoin-hash some day

@fanatid fanatid closed this May 13, 2016
@fanatid
Copy link
Member

fanatid commented May 19, 2016

JFYI, start working on experimental library: addon with support ripemd160,sha1,sha256
https://github.com/fanatid/bitcoinjs-hash
@dcousens if you want we can move this to @bitcoinjs

@axic
Copy link
Contributor

axic commented May 19, 2016

Is this written from scratch or an extension to secp256k1?

@fanatid
Copy link
Member

fanatid commented May 19, 2016

@axic I'm planning write from scratch, not as extension of secp256k1..

@fanatid
Copy link
Member

fanatid commented May 21, 2016

@chjj with what buffer size you tested sha256/hash256? I receive interesting numbers..
On small input size addon is faster, but with huge buffer (1MB for example, like bitcoin blocks) node faster.
https://github.com/fanatid/bitcoinjs-hash#benchmarks

@chjj
Copy link
Contributor Author

chjj commented May 21, 2016

@fanatid, interesting. I was benchmarking the double sha with small transactions (<1kb). I wasn't testing large chunks of data. Let me see what numbers I get.

This is making me think of considering something like:

if (data.length > 100000)
  return nodesha256(data);
return secp256k1sha(data);

Kind of ridiculous, but it could work.

@chjj
Copy link
Contributor Author

chjj commented Sep 12, 2016

@fanatid I think I figured out what's happening here. It looks like node.js does compile openssl with asm support, so, my function was fast on smaller payloads due to the fact that I wasn't instantiating 2 js objects like I would be with the node crypto API. On larger chunks, it looks like the asm wins out even with that handicap.

On a related note, I've finally written bindings for almost everything I need in bitcoin to give bcoin some extra perf: https://github.com/bcoin-org/bcoin-native/tree/master/src

I ended up just using the openssl hash functions.

@fanatid
Copy link
Member

fanatid commented Sep 13, 2016

@chjj agree, openssl asm version can win on large parts of data. On small one one-call functions is faster because they don't create any JS objects and need 1 call instead 3 (create, update, digest).

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.

5 participants