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.

Sign up for GitHub

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

Jump to bottom

Use EntropyRng instead of OsRng #130

Merged
merged 2 commits into from May 6, 2019
Merged

Use EntropyRng instead of OsRng #130

merged 2 commits into from May 6, 2019

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Apr 29, 2019

EntropyRng is also cryptographically secure, but will use jitter RNG (http://www.chronox.de/jent/doc/CPU-Jitter-NPTRNG.html) if the OsRng isn't available.

@tomaka
Copy link
Contributor Author

tomaka commented Apr 29, 2019

The objective of this is paritytech/substrate#2416

@ordian
Copy link
Member

ordian commented May 5, 2019

cc rust-random/rand/issues/760, rust-random/rand/pull/765,

@tomaka
Copy link
Contributor Author

tomaka commented May 6, 2019 •
edited

I suggest merging this anyway.
The reason for this PR is at the moment in rand 0.6 the OsRng doesn't exist for WASM, which leads to the compilation failing altogether.
Whatever changes the discussion in rust-random/rand#760 refer to are, they will only land in rand 0.7, which needs to be manually put in parity-common.

@dvdplm
Copy link
Contributor

dvdplm commented May 6, 2019

@tomaka just to be sure I understand the intent here: OsRng does not compile to wasm; JitterRng does; switching to EntropyRng gets us the needed fallback when compiling for browsers. Correct?

What do you make of the issues @ordian mentions? Seems like EntropyRng is not getting removed in the short term, but will be eventually?

@dvdplm
Copy link
Contributor

dvdplm commented May 6, 2019

LGTM but I'll let @ordian press the green button here first.

@tomaka
Copy link
Contributor Author

tomaka commented May 6, 2019

@tomaka just to be sure I understand the intent here: OsRng does not compile to wasm; JitterRng does; switching to EntropyRng gets us the needed fallback when compiling for browsers. Correct?

Yes

@ordian
Copy link
Member

ordian commented May 6, 2019

I think one of the reasons EntropyRng was deprecated is security concerns (rust-random/rand/issues/699), but it seems like we don't have a better alternative for WASM, so I'll approve this PR.

@tomaka
Copy link
Contributor Author

tomaka commented May 6, 2019

security concerns (rust-random/rand/issues/699)

Oh, that's interesting. I had concerns as well, but considering that it was marked as CryptoRng I was assuming that they knew what they were doing. 😅

@tomaka
Copy link
Contributor Author

tomaka commented May 6, 2019

But as mentioned in rust-random/rand#760, we can assume that the OsRng works everywhere and that the JitterRng will never be used except on WASM.

In other words, this PR is basically a quickfix while waiting for rand 0.7.

@tomaka tomaka merged commit 681faff into paritytech:master May 6, 2019
@tomaka tomaka deleted the osrng-entropyrng branch May 6, 2019 10:00
@tomaka tomaka mentioned this pull request May 6, 2019
Merged
ordian added a commit that referenced this pull request May 6, 2019
@ordian
Merge branch 'master' into dp/feature/add-NumEntries-trait
e708a18 
* master:
  Bump fixed-hash to 0.3.2 (#134)
  Use EntropyRng instead of OsRng (#130)
  Bump parity-crypto to 0.3.1 (#132)
  rust-crypto dependency removal (#85)
  Use newly stablized TryFrom trait for primitive-types (#127)
  change dependencies of `keccak-hash`and `trie-standardmap` (#111)
  bring appveyor back (#118)
  Extract `should_replace` from Scoring and supply pooled txs from same sender (#116)
ordian added a commit that referenced this pull request May 6, 2019
@ordian
Merge branch 'master' into ao-are-you-ready-for-a-good-mul
f7c6964 
* master:
  Bump fixed-hash to 0.3.2 (#134)
  Use EntropyRng instead of OsRng (#130)
  Bump parity-crypto to 0.3.1 (#132)
  rust-crypto dependency removal (#85)
  Use newly stablized TryFrom trait for primitive-types (#127)
ordian added a commit that referenced this pull request May 7, 2019
@ordian
Merge branch 'master' into ao-then-get-ready-for-the-fast-div
c160588 
* master:
  uint: faster mul by u64 (#125)
  fix rocksdb block cache size (#122)
  uint: convert benchmarks to criterion  (#123)
  Bump fixed-hash to 0.3.2 (#134)
  Use EntropyRng instead of OsRng (#130)
  Bump parity-crypto to 0.3.1 (#132)
  rust-crypto dependency removal (#85)
  Use newly stablized TryFrom trait for primitive-types (#127)
@ordian ordian mentioned this pull request Sep 5, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ordian ordian ordian approved these changes

@sorpaas sorpaas sorpaas approved these changes

Assignees
No one assigned
Labels
A0-pleasereview
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tomaka @ordian @dvdplm @sorpaas