STR-503: use `OsRng` consistently

AaronFeickert commented 1 month ago

Description

Switches all uses of thread_rng() to OsRng. Standardizes existing uses of OsRng.

Type of Change

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature/Enhancement (non-breaking change which adds functionality or enhances an existing one)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] Documentation update
[ ] Refactor

Checklist

[x] I have performed a self-review of my code.
[x] I have commented my code where necessary.
[x] I have updated the documentation if needed.
[x] My changes do not introduce new warnings.
[ ] I have added tests that prove my changes are effective or that my feature works.
[x] New and existing tests pass with my changes.

Related Issues

None.

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 83.72093% with 7 lines in your changes missing coverage. Please review.

Project coverage is 57.12%. Comparing base (31cab87) to head (8817f4a). Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
bin/strata-cli/src/recovery.rs	0.00%	2 Missing :warning:
bin/strata-cli/src/seed.rs	0.00%	2 Missing :warning:
bin/strata-cli/src/cmd/change_pwd.rs	0.00%	1 Missing :warning:
bin/strata-cli/src/cmd/faucet.rs	0.00%	1 Missing :warning:
crates/primitives/src/l1.rs	75.00%	1 Missing :warning:

@@            Coverage Diff             @@
##             main     #396      +/-   ##
==========================================
+ Coverage   57.10%   57.12%   +0.01%     
==========================================
  Files         255      255              
  Lines       26978    26967      -11     
==========================================
- Hits        15407    15406       -1     
+ Misses      11571    11561      -10

Files with missing lines	Coverage Δ
bin/strata-client/src/extractor.rs	`94.98% <100.00%> (-1.98%)`	:arrow_down:
crates/bridge-sig-manager/src/operations.rs	`96.70% <100.00%> (ø)`
crates/bridge-tx-builder/src/operations.rs	`96.42% <100.00%> (ø)`
crates/btcio/src/writer/builder.rs	`98.03% <100.00%> (ø)`
crates/consensus-logic/src/reorg.rs	`94.54% <100.00%> (-0.02%)`	:arrow_down:
crates/crypto/src/lib.rs	`92.50% <100.00%> (-0.19%)`	:arrow_down:
crates/evmexec/src/el_payload.rs	`98.64% <100.00%> (ø)`
crates/evmexec/src/engine.rs	`65.52% <100.00%> (-0.15%)`	:arrow_down:
crates/primitives/src/bridge.rs	`86.30% <100.00%> (ø)`
crates/primitives/src/hash.rs	`100.00% <100.00%> (ø)`
... and 14 more

... and 3 files with indirect coverage changes

AaronFeickert commented 1 month ago

I've never seen any issued raised with thread safety of OsRng, but don't have a definitive answer. It was actually considered for direct use in thread_rng, but rejected for performance reasons that shouldn't apply here. (It's also used for thread_rng reseeding.)

I agree that the use of imports for OsRng is preferable. The initial goal was to minimize changes for easier review, but you're right that this is a good time to shore things up.

One additional change that I made at one point was to prefer direct use of fill_bytes (provided by RngCore) for constructing random byte arrays, instead of relying on gen (provided by Rng) for a standard distribution. I have no particular reason to distrust the distribution-based approach, but my spidey sense tells me that the more direct and obvious byte-filling approach is a more conservative approach. When it comes to critical randomness, best not to mess with anything...

storopoli commented 1 month ago

Concur with suggestions but looks good.

wait for the rebase and the upcoming changes before merging

delbonis commented 1 month ago

I would expect it to be thread safe, it'd be weird if it wasn't.

delbonis commented 1 month ago

@storopoli Are suggestions applied?

AaronFeickert commented 1 month ago

I would expect it to be thread safe, it'd be weird if it wasn't.

If it isn't thread safe, much (most?) of the Rust cryptographic ecosystem is in trouble...

alpenlabs / strata