Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

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

Merged
merged 15 commits into from
Jun 17, 2019
Merged

Conversation

dvdplm
Copy link
Collaborator

@dvdplm dvdplm commented Jun 3, 2019

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).

@dvdplm dvdplm self-assigned this Jun 3, 2019
@dvdplm dvdplm added the A0-pleasereview 🤓 Pull request needs code review. label Jun 3, 2019
…ypto-from-devp2p

* dp/chore/devp2p-cosmetics:
  Reorg imports
  Upgrade ethereum types (#10670)
* master:
  [devp2p] Fix warnings and re-org imports (#10710)
Copy link
Contributor

@cheme cheme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

util/network-devp2p/src/connection.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/connection.rs Outdated Show resolved Hide resolved
util/network-devp2p/src/connection.rs Outdated Show resolved Hide resolved
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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did we actually do that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

cheme commented Jun 17, 2019

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
Copy link
Collaborator Author

dvdplm commented Jun 17, 2019

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
Copy link
Collaborator Author

dvdplm commented Jun 17, 2019

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
Copy link
Collaborator Author

dvdplm commented Jun 17, 2019

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.

@dvdplm dvdplm mentioned this pull request Jun 17, 2019
@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust. labels Jun 17, 2019
@ordian ordian removed the A0-pleasereview 🤓 Pull request needs code review. label Jun 17, 2019
@ordian ordian added this to the 2.6 milestone Jun 17, 2019
@ordian ordian merged commit 2d693be into master Jun 17, 2019
@ordian 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
* master:
  [devp2p] Don't use `rust-crypto` (#10714)
dvdplm added a commit that referenced this pull request Jun 18, 2019
* 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)
dvdplm added a commit that referenced this pull request Jun 19, 2019
…-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)
dvdplm added a commit that referenced this pull request Jun 19, 2019
…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)
dvdplm added a commit that referenced this pull request Jun 19, 2019
…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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ethcore-network-devp2p] get rid of rust-crypto dependency
3 participants