[devp2p] Don't use `rust-crypto` #10714

dvdplm · 2019-06-03T12:59:31Z

~~Builds on #10710 so please review that first to reduce noise~~

Fixes #10698 and is essentially a copy&paste from @cheme s "crypto-only` branch (master...cheme:crypto-only).

Attempts to kick the crypto debate forward by replacing rust-crypto with parity-crypto (which in turn uses the "mini crates" from RustCrypto). While much of the same criticism of rust-crypto still applies, I still think we're better off with code that is maintained than being stuck on code from 2016.

Using parity-crypto may also help to keep our crypto code in one place (and @cheme branch goes a lot further in this, but I think it's good to do this step-by-step, so expect further PRs in this sense).

Truncate payload properly

* master: Upgrade ethereum types (#10670)

…ypto-from-devp2p * dp/chore/devp2p-cosmetics: Reorg imports Upgrade ethereum types (#10670)

* master: [devp2p] Fix warnings and re-org imports (#10710)

cheme

Looks good, I let a few comment but not really needed.

util/network-devp2p/src/connection.rs

cheme · 2019-06-14T16:38:19Z

util/network-devp2p/src/connection.rs

+		let encoder = AesCtr256::new(&key_material[32..64], &NULL_IV)?;
+		let decoder = AesCtr256::new(&key_material[32..64], &NULL_IV)?;
+        let key_material_keccak = keccak(&key_material);
+        (&mut key_material[32..64]).copy_from_slice(key_material_keccak.as_bytes());


This last copy seems useless (except to show a repeating pattern with previous lines, which can make sense and then we rely on optimization). Not sure if it is worth removing.

Yeah it seems like we could use key_material_keccak everywhere we use key_material[32..64] below. I'll leave it as it is because I don't understand it fully.

ordian

I don't know what are the performance implications of that change? And can it be a bottleneck really?

util/network-devp2p/src/connection.rs

ordian · 2019-06-17T12:41:59Z

util/network-devp2p/src/connection.rs

 		header.append_raw(&[(len >> 16) as u8, (len >> 8) as u8, len as u8], 1);
 		header.append_raw(&[0xc2u8, 0x80u8, 0x80u8], 1);
-		//TODO: get rid of vectors here


did we actually do that?

we could put packet as a field of this struct, that would be pretty straightforward, but I am really not sure about the lifetime of this one (and if we want to keep memory of unusually large packet allocated).

I removed the comment because like @cheme I was unsure of what the original author of the comment actually intended with it: is to be read like "it sure would be nice to allocate a bit less here" or "this is a terrible performance problem". Such comments seem unhelpful to me so I removed it.

* master: updater: fix static id hashes initialization (#10755) Use fewer threads for snapshotting (#10752) Die error_chain, die (#10747) Fix deprectation warnings on nightly (#10746) fix docker tags for publishing (#10741) DevP2p: Get node IP address and udp port from Socket, if not included in PING packet (#10705) ethcore: enable ECIP-1054 for classic (#10731) Stop breaking out of loop if a non-canonical hash is found (#10729) Refactor Clique stepping (#10691) Use RUSTFLAGS to set the optimization level (#10719) SecretStore: non-blocking wait of session completion (#10303) removed secret_store folder (#10722) SecretStore: expose restore_key_public in HTTP API (#10241) Revert "enable lto for release builds (#10717)" (#10721) enable lto for release builds (#10717) Merge `Notifier` and `TransactionsPoolNotifier` (#10591)

cheme · 2019-06-17T13:09:06Z

My first impression is that this part of the code is not really a bottleneck. A good test would be to try to compare a standard build of this fork with a build with aes-ni activated (since aes-ni activation really show some huge performance changes).
Edit: activating aes-ni is pretty straightforward you just need to pass the -C feature to cargo in order to activate this https://github.com/RustCrypto/block-ciphers/blob/7602f0b8523641f7d17df037736060f2dd0587b9/aes/aes/Cargo.toml#L18 (but making it a default for parity build is another question: it would break compatibility of old cpu (a runtime check is possible but not as straightforward).

dvdplm · 2019-06-17T13:19:11Z

I don't know what are the performance implications of that change? And can it be a bottleneck really?

I do not think so but the truth is we just don't know much about how the network layer is performing, or how that performance impacts the overall performance of the client in general (i.e. is a 10% slowdown or speedup a 1% slowdown or speedup in "normal" operation? more? less?).

The purpose of the PR is to slooooowly start tidying up our usage of crypto and use parity-crypto as much as possible. @cheme did a lot of more good work on this but we sort of lost sight of it and it became (somewhat) stale. My plan was to revive that work piece by piece and this was the first step.

dvdplm · 2019-06-17T13:30:07Z

with aes-ni activated

My understanding was that this was activated autiomatically (Crate switches between implementations automatically at compile time. (i.e. it does not use run-time feature detection)), but maybe I'm wrong. Hmm.

dvdplm · 2019-06-17T14:47:19Z

The aesni crate is not activated by default or automatically. We need to build the whole client with the proper RUSTFLAGS or switch to runtime detection with the is_x86_feature_detected!("aes") macro.

* master: [devp2p] Don't use `rust-crypto` (#10714)

* master: Enable aesni (#10756) remove support of old SS db formats (#10757) [devp2p] Don't use `rust-crypto` (#10714) updater: fix static id hashes initialization (#10755) Use fewer threads for snapshotting (#10752) Die error_chain, die (#10747) Fix deprectation warnings on nightly (#10746) fix docker tags for publishing (#10741) DevP2p: Get node IP address and udp port from Socket, if not included in PING packet (#10705) ethcore: enable ECIP-1054 for classic (#10731) Stop breaking out of loop if a non-canonical hash is found (#10729) Refactor Clique stepping (#10691) Use RUSTFLAGS to set the optimization level (#10719) SecretStore: non-blocking wait of session completion (#10303) removed secret_store folder (#10722) SecretStore: expose restore_key_public in HTTP API (#10241) Revert "enable lto for release builds (#10717)" (#10721) enable lto for release builds (#10717) Merge `Notifier` and `TransactionsPoolNotifier` (#10591)

…-even * master: [devp2p] Update to 2018 edition (#10716) Add a way to signal shutdown to snapshotting threads (#10744) Enable aesni (#10756) remove support of old SS db formats (#10757) [devp2p] Don't use `rust-crypto` (#10714) updater: fix static id hashes initialization (#10755) Use fewer threads for snapshotting (#10752) Die error_chain, die (#10747) Fix deprectation warnings on nightly (#10746) fix docker tags for publishing (#10741) DevP2p: Get node IP address and udp port from Socket, if not included in PING packet (#10705) ethcore: enable ECIP-1054 for classic (#10731)

…p/chore/aura-log-validator-set-in-epoch-manager * dp/chore/aura-warn-when-validators-is-1-or-even: [devp2p] Update to 2018 edition (#10716) Add a way to signal shutdown to snapshotting threads (#10744) Enable aesni (#10756) remove support of old SS db formats (#10757) [devp2p] Don't use `rust-crypto` (#10714) updater: fix static id hashes initialization (#10755) Use fewer threads for snapshotting (#10752) Die error_chain, die (#10747) Fix deprectation warnings on nightly (#10746) fix docker tags for publishing (#10741) Update ethcore/src/engines/validator_set/simple_list.rs DevP2p: Get node IP address and udp port from Socket, if not included in PING packet (#10705) ethcore: enable ECIP-1054 for classic (#10731)

…dp/fix/prevent-building-block-on-top-of-same-parent * dp/chore/aura-log-validator-set-in-epoch-manager: [devp2p] Update to 2018 edition (#10716) Add a way to signal shutdown to snapshotting threads (#10744) Enable aesni (#10756) remove support of old SS db formats (#10757) [devp2p] Don't use `rust-crypto` (#10714) Update ethcore/src/engines/validator_set/simple_list.rs

dvdplm added 6 commits June 1, 2019 19:44

Run cargo fix

3efca47

Optimize imports

431eccf

compiles

95a958d

cleanup

66f3659

Use Secret to store mac-key

be06f57

Truncate payload properly

cleanup

9634be3

dvdplm self-assigned this Jun 3, 2019

dvdplm added the A0-pleasereview 🤓 Pull request needs code review. label Jun 3, 2019

dvdplm added 5 commits June 3, 2019 15:38

Merge branch 'master' into dp/chore/devp2p-cosmetics

d5d1eb0

* master: Upgrade ethereum types (#10670)

Reorg imports

1e68071

Merge branch 'dp/chore/devp2p-cosmetics' into dp/chore/remove-rust-cr…

6f51390

…ypto-from-devp2p * dp/chore/devp2p-cosmetics: Reorg imports Upgrade ethereum types (#10670)

Merge branch 'master' into dp/chore/remove-rust-crypto-from-devp2p

b0c89b8

* master: [devp2p] Fix warnings and re-org imports (#10710)

brwchk hand waving

f190a63

dvdplm mentioned this pull request Jun 4, 2019

[devp2p] Update to 2018 edition #10716

Merged

dvdplm requested review from ordian and cheme June 14, 2019 09:11

cheme approved these changes Jun 14, 2019

View reviewed changes

Review feedback

0ae675b

ordian reviewed Jun 17, 2019

View reviewed changes

whitespace

e4e00c9

error chain is dead

b2e51bb

dvdplm mentioned this pull request Jun 17, 2019

Enable aesni #10756

Merged

ordian approved these changes Jun 17, 2019

View reviewed changes

ordian added A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust. labels Jun 17, 2019

ordian removed the A0-pleasereview 🤓 Pull request needs code review. label Jun 17, 2019

ordian added this to the 2.6 milestone Jun 17, 2019

ordian merged commit 2d693be into master Jun 17, 2019

ordian deleted the dp/chore/remove-rust-crypto-from-devp2p branch June 17, 2019 16:43

dvdplm added a commit that referenced this pull request Jun 18, 2019

Merge branch 'master' into dp/chore/turn-on-aesni

4a38f5c

* master: [devp2p] Don't use `rust-crypto` (#10714)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[devp2p] Don't use `rust-crypto` #10714

[devp2p] Don't use `rust-crypto` #10714

dvdplm commented Jun 3, 2019 •

edited

cheme left a comment

cheme Jun 14, 2019

dvdplm Jun 14, 2019

ordian left a comment

ordian Jun 17, 2019

cheme Jun 17, 2019

dvdplm Jun 17, 2019

cheme commented Jun 17, 2019 •

edited

dvdplm commented Jun 17, 2019

dvdplm commented Jun 17, 2019

dvdplm commented Jun 17, 2019

[devp2p] Don't use rust-crypto #10714

[devp2p] Don't use rust-crypto #10714

Conversation

dvdplm commented Jun 3, 2019 • edited

cheme left a comment

Choose a reason for hiding this comment

cheme Jun 14, 2019

Choose a reason for hiding this comment

dvdplm Jun 14, 2019

Choose a reason for hiding this comment

ordian left a comment

Choose a reason for hiding this comment

ordian Jun 17, 2019

Choose a reason for hiding this comment

cheme Jun 17, 2019

Choose a reason for hiding this comment

dvdplm Jun 17, 2019

Choose a reason for hiding this comment

cheme commented Jun 17, 2019 • edited

dvdplm commented Jun 17, 2019

dvdplm commented Jun 17, 2019

dvdplm commented Jun 17, 2019

[devp2p] Don't use `rust-crypto` #10714

[devp2p] Don't use `rust-crypto` #10714

dvdplm commented Jun 3, 2019 •

edited

cheme commented Jun 17, 2019 •

edited