remove rustc-serialize (#359) #386

cheme · 2018-08-02T14:07:25Z

In this commit I replace rust-serialize with RustSerialize project aes-ctr crate.
Fixes #383
Note that to avoid any api changes, I do not transmit a possible encoding error (the error seems a bit unlikely to occurs : end of keystream), it will likely panic on failure.

This should also fix #371 but it needs to be tested.

To use aes native instruction, the following rust-flags must be use RUSTFLAGS="-C target-feature=+aes,+ssse3" RUSTDOCFLAGS="-C target-feature=+aes,+ssse3".

I also add a feature 'aes-all' for including both version of the aes lib (aes-ni or aes-soft) and choose at runtime (with 'is_86_feature_enabled' macro) which one to use.
I do not believe if there is still a lot of x86 without support for aes or ssse3 and this feature may not be usefull (not activated by default and I would understand if we want to get rid of it).

Feature gated behind 'aes-all'. Building requires RUSTFLAGS="-C target-feature=+aes,+ssse3" and RUSTDOCFLAGS="-C target-feature=+aes,+ssse3". Only support x86 case.

RUSTC aes and sse are enabled). To build with aes-all, aes and sse flag should not be set.

gnunicorn · 2018-08-09T13:19:17Z

secio/Cargo.toml

@@ -14,7 +14,7 @@ protobuf = "2.0.2"
 rand = "0.3.17"
 ring = { version = "0.12.1", features = ["rsa_signing"] }
 aes-ctr = "0.1.0"
-aes-soft = { version = "0.2", optional = true }
+aesni = { git="https://github.com/cheme/block-ciphers.git", features = ["nocheck"], optional = true }


@cheme is there anything in the git-repo that isn't in the release on crates.io? Or why can't we use that?

Yes there was a little change, but RustCrypto guys merge it: RustCrypto/block-ciphers#22 , so we can now use the crates.io dependancy (I change it).

checks).

gnunicorn · 2018-08-10T14:33:01Z

@cheme thanks heaps. Could you rebase this on master, so the win32 tests will be included?

tomaka · 2018-08-10T15:05:28Z

Anyone should be able to merge master thanks to github's button.

Feature gated behind 'aes-all'. Building requires RUSTFLAGS="-C target-feature=+aes,+ssse3" and RUSTDOCFLAGS="-C target-feature=+aes,+ssse3". Only support x86 case.

RUSTC aes and sse are enabled). To build with aes-all, aes and sse flag should not be set.

checks).

… RustCrypto_switch

cheme · 2018-08-10T15:48:50Z

win32 pass 👍

tomaka · 2018-08-12T15:06:34Z

secio/src/stream_cipher.rs

@@ -0,0 +1,109 @@
+


Missing copyright header.

tomaka · 2018-08-12T15:07:14Z

secio/src/stream_cipher.rs

+
+/// Returns your stream cipher depending on `KeySize`.
+#[cfg(not(all(feature = "aes-all", any(target_arch = "x86_64", target_arch = "x86"))))]
+pub(crate) fn ctr(key_size: KeySize, key: &[u8], iv: &[u8]) -> Box<StreamCipherCore + 'static> {


Doesn't need to be pub(crate).

tomaka · 2018-08-12T15:10:34Z

secio/src/stream_cipher.rs

+    feature = "aes-all", 
+    any(target_arch = "x86_64", target_arch = "x86"),
+))]
+#[test]


Should be in a mod tests module.

tomaka · 2018-08-12T15:11:33Z

secio/src/codec/encode.rs

-        // Apparently this is the fastest way of doing.
-        // See https://gist.github.com/kirushik/e0d93759b0cd102f814408595c20a9d0
-        let mut out_buffer = BytesMut::from(vec![0; capacity]);
+        let mut data_buf = item;


Why this line, instead of renaming the function parameter or using item in the code below?

Useless (just wanted to keep proto looks but does not need to be here).

tomaka · 2018-08-12T15:15:09Z

secio/src/stream_cipher.rs

+/// Returns your stream cipher depending on `KeySize`.
+#[cfg(all(feature = "aes-all", any(target_arch = "x86_64", target_arch = "x86")))]
+pub(crate) fn ctr(key_size: KeySize, key: &[u8], iv: &[u8]) -> Box<StreamCipherCore + 'static> {
+    if *aes_alt::AES_NI {


I haven't really digged into the crate, but from what I understand the aes_ctr crate already does that, no?
It would really really really be great if we didn't have platform-specific code.

aes_ctr run compile time switch between NI and soft implementation (to activate NI only crate flags from original PR msg should be use). A runtime check is not in aes_ctr crate, that is what eas-all feature does, but honestly I doubt that there is a lot of x86 cpu that could not run NI (I did my tests with qemu).
The best thing to do would be to try to PR runtime NI check to RustCrypto (not directly this code as dynamic type usage does not fit RustCrypto crates design), not sure if it would be easily accepted.
Another option would be to simply remove the 'aes-all' feature as I do not know if it is wanted (usecase would be single x86 build supporting old x86 cpu, it seems reasonable to me to defaults to NI activated build (binary distribution) and ask users to build from source if they are using x86 cpu that does not support aes NI and sse3). For info in #371 Sergei low end config could run aes-ni.

tomaka · 2018-08-12T15:16:22Z

Haven't had time to review.

This reverts commit 7399688.

…bp2p#466)" This reverts commit 0a3d4cd.

This reverts commit 0a3d4cd.

cheme added 2 commits August 2, 2018 10:53

Replace rust-crypto dependency by RustCrypto project crate aes-ctr

66193eb

Support for x86 build supporting both aesni and aessoft at runtime.

c4a4a86

Feature gated behind 'aes-all'. Building requires RUSTFLAGS="-C target-feature=+aes,+ssse3" and RUSTDOCFLAGS="-C target-feature=+aes,+ssse3". Only support x86 case.

gnunicorn added pr-needs-work labels Aug 2, 2018

cheme added 2 commits August 6, 2018 11:57

Change CI to build with aes ni on all-features

8537e96

Switch aes-all logic (is_x86 runtime test did not work as I thought when

bc9dd63

RUSTC aes and sse are enabled). To build with aes-all, aes and sse flag should not be set.

gnunicorn added pr-needs-review and removed ci-failed labels Aug 8, 2018

gnunicorn reviewed Aug 9, 2018

View reviewed changes

Use latest aesni from crates.io (with pr to disable compile time

25f91c0

checks).

gnunicorn approved these changes Aug 10, 2018

View reviewed changes

gnunicorn added pr-needs-rebase and removed pr-needs-review labels Aug 10, 2018

Merge branch 'master' into RustCrypto_switch

f8da503

ghost assigned tomaka Aug 10, 2018

ghost added the in progress label Aug 10, 2018

cheme and others added 7 commits August 10, 2018 17:25

Replace rust-crypto dependency by RustCrypto project crate aes-ctr

cfb9990

Support for x86 build supporting both aesni and aessoft at runtime.

d6b1a3e

Feature gated behind 'aes-all'. Building requires RUSTFLAGS="-C target-feature=+aes,+ssse3" and RUSTDOCFLAGS="-C target-feature=+aes,+ssse3". Only support x86 case.

Change CI to build with aes ni on all-features

7c8f52e

Switch aes-all logic (is_x86 runtime test did not work as I thought when

6fb5fd1

RUSTC aes and sse are enabled). To build with aes-all, aes and sse flag should not be set.

Use latest aesni from crates.io (with pr to disable compile time

2d0f8d6

checks).

Merge branch 'RustCrypto_switch' of github.com:cheme/rust-libp2p into…

edf1d69

… RustCrypto_switch

Merge branch 'master' into RustCrypto_switch

eb2a9d5

ghost assigned gnunicorn Aug 10, 2018

gnunicorn added pr-queued-to-merge and removed pr-needs-rebase labels Aug 10, 2018

gnunicorn merged commit 7399688 into libp2p:master Aug 10, 2018

ghost removed the in progress label Aug 10, 2018

gnunicorn removed pr-good-waiting-for-ci labels Aug 10, 2018

tomaka reviewed Aug 12, 2018

View reviewed changes

cheme mentioned this pull request Aug 14, 2018

Complements to #389 #415

Merged

tomaka added a commit to tomaka/libp2p-rs that referenced this pull request Sep 7, 2018

Revert "remove rustc-serialize (libp2p#359) (libp2p#386)"

aa99b28

This reverts commit 7399688.

tomaka added a commit that referenced this pull request Sep 7, 2018

Revert "remove rustc-serialize (#359) (#386)" (#466)

0a3d4cd

This reverts commit 7399688.

tomaka added a commit to tomaka/libp2p-rs that referenced this pull request Sep 13, 2018

Revert "Revert "remove rustc-serialize (libp2p#359) (libp2p#386)" (li…

4cc6452

…bp2p#466)" This reverts commit 0a3d4cd.

tomaka added a commit that referenced this pull request Sep 17, 2018

Revert "Revert "remove rustc-serialize (#359) (#386)" (#466)" (#477)

d719ac6

This reverts commit 0a3d4cd.

tomaka mentioned this pull request Oct 12, 2018

remove rustc-serialize #359

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove rustc-serialize (#359) #386

remove rustc-serialize (#359) #386

cheme commented Aug 2, 2018

gnunicorn Aug 9, 2018

cheme Aug 9, 2018

gnunicorn commented Aug 10, 2018

tomaka commented Aug 10, 2018

cheme commented Aug 10, 2018

tomaka Aug 12, 2018

tomaka Aug 12, 2018

cheme Aug 13, 2018

tomaka Aug 12, 2018

tomaka Aug 12, 2018

cheme Aug 13, 2018

tomaka Aug 12, 2018

cheme Aug 13, 2018

tomaka commented Aug 12, 2018

remove rustc-serialize (#359) #386

remove rustc-serialize (#359) #386

Conversation

cheme commented Aug 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnunicorn commented Aug 10, 2018

tomaka commented Aug 10, 2018

cheme commented Aug 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomaka commented Aug 12, 2018