Update rust-crypto dependency to RustCrypto project#14
Conversation
Note that we use RustCrypto aes root crates : so if we build with RUSTFLAGS='-C target-feature=+aes,+ssse3,+sse2' on x86, AES native instruction will be used. Also added two Aes encryptor struct in preparation of switching (only two use case remaining at the time in 'network-devp2p' crate.
debris
left a comment
debris
left a comment
There was a problem hiding this comment.
We definitely need to switch from rust-crypto to something else, but I'm not convinced that we should go with RustCrypto. There are several things that I don't like about it:
-
the structure; Currently we use a single crypto library. Replacing it with 6 other is a maintenance overhead. Every time one of those libraries is updated, we should go to the repo and check the code that has been changed since the last release
-
lack of users; There is no guarantee that this libraries will be maintained a few months from now
-
the interface; I quite lack the consistency and low-level interface of rust-crypto. I don't know why it was changed, but seeing additional
copy_from_slicecalls makes me wonder if the performance of those two libraries is the same. Without benchmarks and a proof that it is, we cannot merge it
|
I agree with your first two points, this PR is mainly an update (RustCrypto is mainly rust-crypto with maintenance and some evo). On the third point we can still use a similar interface as rust-crypto (I just change to avoid usage of two buffers so we would need to copy_from_slice too at some places), yet the stream interface leads to better results in my case (no need to adjust buffer size to a block length multiple). About the As you asked I did a very quick bench : https://gist.github.com/cheme/2dcd97a7a08efc73027126f70fb3e64a There is some small difference (I think instantiation may be a bit more costy with RustCrypto : for small content it is slower while for big content it is way faster). I am keeping it for now (it was in libp2p that rust-crypto was a real issue : libp2p/rust-libp2p#371 ), as it is not that big of an issue to drop it but other. Also about the choice of a crypto crate, I do not believe there is a magic solution, yet I think that using standard traits could be a good thing (being able to switch crypto implementation easily, especially when targeting wasm or other less common targets). |
@cheme I've seen this idea floating around in a few places and it sounds very enticing (especially being able to have a fully pluggable crypto backend so we can run tests against all backends) but I'm a bit hazy on the details of how this would work exactly. Do you have a clear picture of the set of traits we'd need? Maybe even an example? Would be curious to see it. :) |
|
I think we should ask @tomaka about that (I believe there was some issues using it with libp2p use and we may want his feedback). |
Remove unused scrypt features.
|
[clabot:check] |
|
It looks like @cheme signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
1 similar comment
|
It looks like @cheme signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
Uint ASM only with feature
Uint ASM only with feature
|
Superseded by #85. |
7 participants
In this PR I got rid of the remaining uses of the rust-crypto crate, to use the corresponding RustCrypto ones.
One major difference is that I do not target aes-safe but the roots aes crates : if compiling with
RUSTFLAGS='-C target-feature=+aes,+ssse3,+sse2'the aesni implementation will be used (only for x86).There is also two aes encoder/decoder struct which are basically here to get rid of rust-crypto in network-devp2p.