Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace sodium with ed25519_dalek and sha2 #1816

Merged
merged 10 commits into from Dec 5, 2019
Merged

Conversation

@ailisp
Copy link
Member

ailisp commented Dec 4, 2019

#1656
A side benefit of this it's ed25519_dalek default built without avx2, which is known to illegal instruction in CPU as old as i7-3xxx and some old e3/e5 cpus

@ailisp ailisp requested a review from abacabadabacaba Dec 4, 2019
@nearmax
nearmax approved these changes Dec 4, 2019
Copy link
Collaborator

nearmax left a comment

Good stuff. Do we know how it affects our benchmarks?

core/crypto/src/signature.rs Show resolved Hide resolved
runtime/near-vm-logic/Cargo.toml Outdated Show resolved Hide resolved
@@ -5,7 +5,9 @@ authors = ["Near Inc <hello@nearprotocol.com>"]
edition = "2018"

[dependencies]
sodiumoxide = "0.2.5"
ed25519-dalek = "0.9.1"
rand_os = "0.1"

This comment has been minimized.

Copy link
@bowenwang1996

bowenwang1996 Dec 4, 2019

Member

Where is this used?

This comment has been minimized.

Copy link
@ailisp

ailisp Dec 4, 2019

Author Member

In https://github.com/nearprotocol/nearcore/blob/replace-sodium/core/crypto/src/signature.rs#L328

You may be able to construct a rng to used here, but I couldn't so I borrow how ed25519-dalek doc/tests do this.

This comment has been minimized.

Copy link
@nearmax

nearmax Dec 4, 2019

Collaborator

"0.1" is an outdated version for it https://crates.io/crates/rand_os
Also, it is deprecated.

This crate is deprecated: OsRng is available in rand_core since version 0.5.1.

This comment has been minimized.

Copy link
@ailisp

ailisp Dec 4, 2019

Author Member

I tried a few more options, seems no one other than the depreciated rand_os works. (ed25519_dalek also uses rand_os="0.1" as dep, I can submit an issue to ask them to upgrade for 1.0. They're in 1.0-pre3 now)

In detail:

  1. rand_core::OsRng::default(), rand_core=0.5 doesn't work because
                let keypair = ed25519_dalek::Keypair::generate::<sha2::Sha512, _>(&mut csprng);

gives error

the trait bound `rand_core::os::OsRng: rand_core::CryptoRng` is not satisfied
the trait `rand_core::CryptoRng` is not implemented for `rand_core::os::OsRng`
  1. rand_core::OsRng doesn't exist in rand_core=0.3

  2. rand::rngs::OsRng::default() gives same error as 1

The most weird part is rand_core::CryptoRng does implemented for rand_core::OsRng:
image

I suspect the reason is ed25519_dalek use rand_core=0.3 and rand_os=0.1:
https://github.com/dalek-cryptography/ed25519-dalek/blob/master/Cargo.toml#L33
And the OsRng trait it need for Keypair::generate needs to satisfy rand_core 0.3's CryptoRng. However, 1) rand_core 0.3 doesn't have OsRng, 2) rand_core 0.5/rand 0.6 does have OsRng, but it impls rand_core 0.5's CryptoRng.

The only way I can think of to fix is fix in upstream dalek. Our near-crypto uses rand_core 0.3 by the way, probably should update to 0.5 too.

@@ -5,7 +5,9 @@ authors = ["Near Inc <hello@nearprotocol.com>"]
edition = "2018"

[dependencies]
sodiumoxide = "0.2.5"
ed25519-dalek = "0.9.1"
rand_os = "0.1"

This comment has been minimized.

Copy link
@nearmax

nearmax Dec 4, 2019

Collaborator

"0.1" is an outdated version for it https://crates.io/crates/rand_os
Also, it is deprecated.

This crate is deprecated: OsRng is available in rand_core since version 0.5.1.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 4, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (staging@cfde1f5). Click here to learn what that means.
The diff coverage is 85.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##             staging    #1816   +/-   ##
==========================================
  Coverage           ?   78.45%           
==========================================
  Files              ?      171           
  Lines              ?    43611           
  Branches           ?        0           
==========================================
  Hits               ?    34213           
  Misses             ?     9398           
  Partials           ?        0
Impacted Files Coverage Δ
runtime/runtime/src/ext.rs 52.92% <0%> (ø)
runtime/near-vm-logic/src/mocks/mock_external.rs 82.51% <100%> (ø)
core/primitives/src/hash.rs 96.42% <100%> (ø)
core/crypto/src/test_utils.rs 89.79% <100%> (ø)
core/crypto/src/signature.rs 77.95% <84.04%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfde1f5...eaf9142. Read the comment docs.

ailisp added 2 commits Dec 4, 2019
@ailisp ailisp requested a review from nearmax Dec 4, 2019
@ailisp

This comment has been minimized.

Copy link
Member Author

ailisp commented Dec 4, 2019

reported to ed25519-dalek: dalek-cryptography/ed25519-dalek#104 If it's an important concern I can send them a PR too.

Good stuff. Do we know how it affects our benchmarks?

This I don't know, ed25519-dalek's readme in github mentions a few benchmark that looks very fast to me. Do we have/should i write a benchmark for this?

@nearmax
nearmax approved these changes Dec 4, 2019
Copy link
Collaborator

nearmax left a comment

Let's create a tracking issue to get rid of the deprecated crate. Approving to unblock the merge.

@ailisp ailisp added the automerge label Dec 5, 2019
@nearprotocol-bulldozer nearprotocol-bulldozer bot merged commit a50cbc2 into staging Dec 5, 2019
1 check passed
1 check passed
gitlab-ci
Details
@nearprotocol-bulldozer nearprotocol-bulldozer bot deleted the replace-sodium branch Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.