Move sha3 over to optionalDependencies

[davidmurdoch]

[email protected] with its C implementation is about 45x faster than [email protected]'s pure JS implementation, so I think it makes sense to keep 1.2.2 around (see phusion/node-sha3#44 (comment) for benchmarks).

Also, browserify-sha3 uses js-sha3, which is currently the fastest pure-js implementation, so it makes sense to keep it for the browser-fallback.

Additionally, many users installing this library have issues doing so due to [email protected]'s dependency on node-gyp, python, and other missing build tools.

To work around this issue [email protected] can be installed as an optionalDependency, which will allow for the install of sha3 to fail, but the install of keccakjs to still succeed (just like is done with the scrypt dependency in scrypt.js).

[pgebheim]

@axic -- Is it possible that you can review and merge this? Would help us a lot with the AugurProject/augur CI builds.

[davidmurdoch]

I initially closed this in favor of #11, however that PR is for sha3 v2, which is slower than v1. I've reopened so we can figure out if we want to update the sha3 dependency to v2 or not. I'd prefer we get an updated sha3 v1 that keeps its native performance benefits.

[pgebheim]

sha3 v1 comes with the various node-gyp problems. Both v1 and v2 have the same interface, so truely a project can choose whichever suites it. For Augur, we fixed this by specifying the resolutions key of package.json and pointing it to the version we cared about:

https://github.com/AugurProject/augur/blob/master/package.json#L97-L99

[davidmurdoch]

Thanks @pgebheim. Unfortunately we (Ganache and Truffle) need to support npm and yarn, and npm doesn't support resolutions. My preference is to keep the native performance benefits the v1 branch provides, though that seems unlikely.

[pgebheim]

Honestly seems like the easiest solution is to not use keccakjs at all and instead just include the version of sha3 you want in the project if this is an issue -- since all this does is wrap things anyway.

From the perspective of an ecosystem which is currently (for various reasons) importing keccakjs -- I think that moving the JS only is the only sane option ans node-gyp is rife with issues.

If someone really wants native benefits, then they definitely don't need this browser wrapping + browserify, and they should just be installing the [email protected] manually.

[davidmurdoch]

If someone really wants native benefits, then they definitely don't need this browser wrapping + browserify, and they should just be installing the [email protected] manually.

Except [email protected] doesn't work on Node 12, so they can't install it manually.

Also, keccakjs has 25 public npm packages that directly depend on it, and likely thousands that include it as a transitive dependency. Switching to sha3 v2 may slow a lot of packages down rather significantly Since keccakjs is a likely a transitive dependency, package authors may not be able to choose the version they'd like to use anyway.

optionalDependencies is a great way to allow for node-gyp to fail, but the package to still install successfully. ...of course it doesn't matter if we can't get a Node 12 compatible version of [email protected]. :-D

---

With all that said, https://github.com/emn178/js-sha3 is much faster than [email protected], according to sha3's own benchmarks. So maybe we can switch to js-sha3 and not cause too much of a performance hit. Or wait until phusion/node-sha3#51 is closed.

[canterberry]

I'll be working on phusion/node-sha3#51 tomorrow to bring the speed up -- might be some easy wins. It won't be native C speed, but may be comparable to js-sha3.

[pgebheim]

Good idea to get that up to speed -- and in a way it is an aside. And, having a dependency that is pulled in elsewhere that breaks builds consistently (a la node-gyp) is a huge issue. Worse than a potential performance issue. The fact of the matter is all these libs pull this in via a transitive dependency and don't even know what's happening under the hood. Basically none of the packages that currently depend on this library actually need it, as they would all need to do their own browser bundling anyway. The potential benefit of a simple do it all library is mitigated by the fact that it obscures an actual decision. In a sense this library is a schelling point -- and if it is going to be that it needs to be as compatible as possible. My real recommendation is to communicate to library writers not to use this library and choose appropriate Sha3 implementations for their needs -- and better yet to specify them as optional and allow for users to choose the one necessary for their application. Paul

…

________________________________ From: Devin Canterberry <[email protected]> Sent: Tuesday, April 30, 2019 8:19:09 PM To: axic/keccakjs Cc: Paul Gebheim; Mention Subject: Re: [axic/keccakjs] Move `sha3` over to `optionalDependencies` (#7) I'll be working on phusion/node-sha3#51<phusion/node-sha3#51> tomorrow to bring the speed up -- might be some easy wins. It won't be native C speed, but may be comparable to js-sha3. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#7 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAAU76RU3V4LBYRHNYNDRSDPTED23ANCNFSM4GLJZ65Q>.

[MicahZoltu]

I suspect the vast majority of projects depending on this library do not need or even notice the speed that native hashing provides. However, there are tickets all over GitHub of people complaining about node-gyp problems brought about because everything in the ecosystem transitively depends on this one library.

As @pgebheim stated earlier, if you really do need the speed and can use native code then you are running in node (not browser) and you can simply depend on a native implementation directly.

[davidmurdoch]

everything in the ecosystem transitively depends on this one library.
you can simply depend on a native implementation directly

@MicahZoltu When keccakjs is used as a transitive dependency you can't (easily) change the dependency out for a native implementation (especially in projects that don't have a build step).

p.s., to be clear, I really hate that I have to maintain a library that depends on node-gyp working. I just want to ensure that keccakjs doesn't update its implementation (at least not in a sem-minor/patch) version to a library that makes it 45x slower. A contrived example showcasing why upgrading to 2.x could be a issue: if someone was using this library to do server-side password hashing and authentication (where password hashing/verification is generally recommended that it should take at least 500ms) a 45x slowdown could absolutely bring down a busy auth server - even if the server had plenty of overhead.

[MicahZoltu]

Resolving this via a major version bump certainly seems like a fine solution me. It signals to developers that, "big things have changed, make sure you know what you are doing when you upgrade" and it shouldn't get picked up automatically by build systems.

@MicahZoltu When keccakjs is used as a transitive dependency you can't (easily) change the dependency out for a native implementation (especially in projects that don't have a build step).

If your auth system depends on "slow hashing" (I'm curious why you would want this) then I don't think you should be acquiring your slowness via a transitive dependency that doesn't make guarantees about performance. This strikes me as akin to https://xkcd.com/1172/ (though not a perfect fit).

[davidmurdoch]

If your auth system depends on "slow hashing" (I'm curious why you would want this) then I don't think you should be acquiring your slowness via a transitive dependency

It was just a contrived example; but the potential for a similar use case is still there.

---

[It] doesn't make guarantees about performance

The README of this package states:

The only Keccak hash (aka SHA3 before standardisation) library you need in Javascript. Ever. Pinky promise!

and

There's no speed loss, it is as thin as it can get

This package does make performance claims. ¯\(ツ)/¯

---

I'm curious why you would want this

We're getting off topic here... but password-based authentication must be computationally expensive. A lot of work and research goes into to developing key derivation functions that are computationally expensive and can't be easily optimized (pbkdf2, bcrypt, scrypt, etc).

[MicahZoltu]

There's no speed loss, it is as thin as it can get

Ah, that is fair. Perhaps the right angle here @pgebheim is to convince everyone in the ecosystem to stop adding this as a transitive dependency. I have been trying to do this for about a year now, but have not had much success so far (though I also haven't put forth much effort).

[canterberry]

💡 Note that the sha3 package provides this disclaimer:

If considering this library for the purpose of protecting passwords, you may actually be looking for a key derivation function, which can provide much better security guarantees for this use case.

Among those security guarantees would be a highly optimized top-to-bottom implementation capable of providing the assurances expected of a "slow hash", which no conglomerate of libraries would be able to provide quite as well due to the abstraction tax.

Bottom line:

• If having the best possible performance is essential for your use case, such as in the case of a library implementing a slow hash, then you want an optimized implementation in C or assembly with very thin bindings. The trade-off here is that you're typically introducing compilation overhead, dependencies, and risk at install time.

• If portability is essential, then you need a pure JavaScript implementation. The trade-off here is that performance won't be quite as good as a native implementation.

In practice, the overwhelming majority of Keccak usage involves Ethereum and its derivatives. While there are certainly other use cases out there, for this one I know that portability is very important and that other factors influence performance far more greatly than raw hash speed.

[pgebheim]

Ah, that is fair. Perhaps the right angle here @pgebheim is to convince everyone in the ecosystem to stop adding this as a transitive dependency. I have been trying to do this for about a year now, but have not had much success so far (though I also haven't put forth much effort).

Indeed @MicahZoltu -- we can start with ethereumjs-utils.

[davidmurdoch]

[email protected] with support for Node 12 was just released! 🎉