Upgrade to WASM and Rust Node.js addon (N-API). #53
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
junderw
reviewed
Mar 7, 2021
lib/validate.js
Outdated
| ]); | ||
|
|
||
| function isUint8Array(value) { | ||
| return value instanceof Uint8Array; |
There was a problem hiding this comment.
iirc there was an issue in another library where some environments this doesn't work well.
There was a problem hiding this comment.
I remember only this issue -- cryptocoinjs/secp256k1-node#175 🤔 I think that value can be anything while it's correct work with Uint8Array.prototype.set (we copy input value data to WASM input buffer with set).
There was a problem hiding this comment.
Do you think it worth to change to something like?
function itemsInRange(value) {
for (let i = 0; i < value.length; ++i) {
if (value[i] < 0 || value[i] > 0xff) return false;
}
return true;
}
function isUint8ArrayLike(value, length) {
return value instanceof Uint8Array || (
value !== undefined && value !== null && value.length === length && itemsInRange(value)
)
}
junderw
reviewed
Mar 7, 2021
junderw
reviewed
Mar 7, 2021
My crappy wasm-pack WASM which did so many bad inefficient rust things was 153KB... but when it is gzipped it is reduced by size 65%. Maybe some of the optimization tools I used (like wee_alloc and the WASM strippers I used in the docker command etc.) will help. I see you are already using wasm-opt.
|
|
This PR is historic and godlike. Moving JS libraries to depend on libsecp256k1 via a slim interface built from a single rust code base compiled into WASM and NAPI/NEON bindings is a huge leap towards having a "less insecure" JS Bitcoin ecosystem. |
|
5 tasks
|
Finally, first benchmark! Wasm vs js: 2x for sign and pk creation, 4x for verify, etc. full output (linux i5-8250U)full output for Apple M1Rust addon with different `opt-level` benchmarksafter
bench on debian:i686 (VirtualBox) |
|
Need few more things:
but this should not prevent review. Hope that review will not take a long time @junderw 😆 |
|
I am still in the middle of review. One quick fix I would like to see:
|
|
Changed in fanatid#15. |
|
Changed to ES6 import only in fanatid#16 |
3 tasks
|
tACK 364b416 |
|
Leftover tasks:
|
Merged
|
Moved to #55 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WIP... huge upgrade: move from C++ Node.js addon (NAN) and JS (elliptic) to WebAssembly and Rust Node.js addon (
probably napi, or maybe neon?napi).At the moment of creating pull request:
Next steps for moving pull request from the draft:
Remove secp256k1-sys crate. We can use bitcoin-core/secp256k1 directly with minimum additional things. At creation moment optimized release WASM file is 400KB, what is insane.Investigate how to reduce size withsecp256k1-sys. Use patched secp256k1-sys fanatid/tiny-secp256k1#3Add tests to oursecp256k1FFI crate.(, or use optionalDependencies? More benches fanatid/tiny-secp256k1#8profile.releaseinCargo.toml,#![no_std])Add prebuildify for Node.js addon.Node.js (at least v12.0.0) does not support modules great, soAdd test in browser env fanatid/tiny-secp256k1#4importwill be good for browsers, while for Node.js we will need to convert our code torequire.I hope that I can handle all this 😅