ssbc / chloride

MIT License
90 stars 17 forks source link

BREAKING: Replace TweetNaCl and libsodium-wrapper #22

Closed christianbundy closed 1 year ago

christianbundy commented 3 years ago

The TweetNaCl API was underused, and the libsodium-wrapper provider is bigger and slower than necessary. This commit introduces Sodium-JavaScript, which is a hand-written JS + Wasm port of libsodium meant to pair well with Sodium-Native.

See-also: https://github.com/ssb-js/chloride/pull/21 Fixes: https://github.com/ssb-js/chloride/issues/18

arj03 commented 3 years ago

I'm a bit torn here. I checkout out this branch to give it a go. npm i (which has to compile from source, this was fixed in the other branch). Then ran npm test that crashed with a (I think, its quite noisy):

expected throw: Error: weak public key detected
    at exports.crypto_scalarmult (/home/arj/dev/chloride/node_modules/sodium-browserify-tweetnacl/index.js:90:46)
    at module.exports.<computed> (/home/arj/dev/chloride/node_modules/sodium-browserify/index.js:16:25)
    at apply (/home/arj/dev/chloride/node_modules/chloride-test/index.js:41:18)
    at /home/arj/dev/chloride/node_modules/chloride-test/index.js:65:9
    at Array.forEach (<anonymous>)
    at module.exports (/home/arj/dev/chloride/node_modules/chloride-test/index.js:50:9)
    at Object.<anonymous> (/home/arj/dev/chloride/test.js:9:5)
    at Module._compile (internal/modules/cjs/loader.js:1256:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1277:10)
    at Module.load (internal/modules/cjs/loader.js:1105:32)

Going to try the other branch.

christianbundy commented 3 years ago

@arj03 I don't think that's a test failure, I think that's "this was expected to throw and it did". (This is why I switched to Tape in #21, it's very confusing.) Try this:

index 553103c..8095a4f 100644
--- a/test.js
+++ b/test.js
@@ -2,7 +2,7 @@
 var test = require('chloride-test')

 //if(process.title === 'node')
-  process.exit(
+  console.log(
    test(require('./')).fail
   + test(require('./bindings')).fail
   + test(require('./browser')).fail

It should report 0, which is the number of test failures. Can you confirm?

arj03 commented 3 years ago

I'm really tired here, went to bed quite late yesterday because I was working on a new flumelog. So please don't take this the wrong way. It's amazing to see what you have done here and I'd really like to see this in. What I'm trying to say is that I'd rather spend time testing the other branch with all the changes if that is where we end up anyway, then trying to figure out these half-steps.

KyleMaas commented 3 years ago

Any progress on this? I was looking at maybe trying to use sodium-universal, but it seems this is the preferred direction.

christianbundy commented 3 years ago

I think the only problem with this branch is that it's somehow slower, which is weird because each of the functions was faster in my benchmark. (More comments about this in #21 I think.)

It's unclear to me whether there's just a difference in our machines, or maybe one of the benchmarks is inaccurate, or maybe there's a small implementation problem that we could fix to make sodium-javascript work better for our use-case.