Consider removal of `sodium-native` optional dependency, leaving only `tweetnacl`

grempe commented 4 years ago

Is your feature request related to a problem? Please describe.

sodium-native, while significantly faster than tweetnacl, introduces complexity in the form of:

It requires native code compilation tools to be present in the runtime environment
It causes significant complexity when trying to use within a Docker environment
It confuses developers who need to determine if their use-case needs the additional speed.
It may (?) be solving a performance problem that doesn't exist in the real world?
sodium-native has not been put under a security audit to my knowledge (tweetnacl has been formally audited by Cure53 and is FAR less complex code-wise).

Describe the solution you'd like

No additional install-time or compilation time dependencies or decision making.

I've created and run a simple benchmark suite to satisfy my own curiosity as to the speed differences between these two libraries. There is no contest for simple signing operations. sodium-native is far faster. Here is the benchmark repo and the output:

https://github.com/truestamp/ed25519-bench

❯ node benchmark.js
Running "tweetnacl vs. sodium-native" suite...
Progress: 100%

  sign w/ tweetnacl:
    286 ops/s, ±2.24%      | slowest, 99.47% slower

  sign w/ sodium-native:
    54 008 ops/s, ±0.62%   | fastest

Finished 2 cases!
  Fastest: sign w/ sodium-native
  Slowest: sign w/ tweetnacl

Saved to: benchmark/results/sign.json

Saved to: benchmark/results/sign.chart.html

TL;DR : 54,008 vs. 286 operations per second. Crushed it!

However, I am left wondering if there is ever any use case for using js-stellar-base where this difference is meaningful? Would any use case of this library ever require thousands of signing operations per second vs. hundreds of operations/second? Is 3.49ms/signature ever too slow for normal use?

If so, and I am just unaware of these high performance use-cases, then by all means sodium-native should stay as an optional dependency. If not, then I feel it should be removed as it adds needless complexity and it introduces additional operational and security overhead and code complexity. This may fall under the category of pre-mature optimization.

Describe alternatives you've considered

The alternatives are already baked into this library as primary and optional dependencies.

Additional context

cc: @abuiles cc: @dchest

s-a-y commented 3 years ago

+1 to get rid of sodium-native (security concerns) Just want to point out that since Node 12.x ed25519 is supported by Node's crypto library, which also does a pretty decent job, here's my benchmark results

Running "tweetnacl vs. sodium-native vs crypto" suite...
Progress: 100%

  sign w/ tweetnacl:
    154 ops/s, ±10.88%      | slowest, 99.58% slower

  sign w/ sodium-native:
    36 984 ops/s, ±1.05%   | fastest

  sign w/ crypto:
    23 004 ops/s, ±1.88%   | 37.8% slower

Finished 3 cases!
  Fastest: sign w/ sodium-native
  Slowest: sign w/ tweetnacl

grempe commented 3 years ago

FYI, I have incorporated both signing and verification benchmarks into the little test repo with the help of @s-a-y :

https://github.com/truestamp/ed25519-bench

This includes benchmarks for tweetnacl-js, sodium-native, and Node.js crypto sign.

I like the idea of swapping out sodium-native for Node.js native ed25519 signing capability. One of the things that concerned me though was that Node crypto expects its private and public keys to be a KeyObject which is only able to be imported from a pem or der encoded form. This is a PITA, and doesn't easily allow you to setup keys from a 32 byte seed.

However, I found a library that makes easy work of the conversion from a raw byte seed to a keypair in the form that Node crypto expects.

https://github.com/ipfs-shipyard/js-crypto-key-composer

This lib, which is pure JS and has no non-dev dependencies, makes it easy to swap out any use of sodium-native ed25519 sign and verify for the native Node.js crypto lib. I built a little demo compatibility tester which shows that all three of the libs above can be seeded from a single 32 byte value and generate detached signatures that any one of them can also verify.

https://github.com/truestamp/ed25519-bench#compatibility

https://github.com/truestamp/ed25519-bench/blob/master/compat.js

Replacing sodium-native with the Node crypto lib would remove a major dependency (one that even requires a big Warning! in the README.md) in favor of a Node native implementation. The usage of tweetnacl-js can remain unchanged as a fallback.

As noted, ed25519 signing is supported by Node since LTS version 12. It is in fact the oldest LTS release still supported, which means that if this lib stays aligned with those LTS versions there is no compatibility issue. Especially with the build in fallback to a native JS lib.

grempe commented 2 years ago

I have created a pull request for review which removes the optional dependency on sodium-native (leaving tweetnacl-js in place to handle all sign/verify functions just as it previously did). I would appreciate any comments to see if we can have this merged.

https://github.com/stellar/js-stellar-base/pull/495

mahnunchik commented 5 months ago

Please have a look at https://github.com/paulmillr/noble-curves alternative to replace both sodium-native and tweetnacl.

stellar / js-stellar-base

Consider removal of `sodium-native` optional dependency, leaving only `tweetnacl` #339