-
Notifications
You must be signed in to change notification settings - Fork 339
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
Use chacha in get_secure_random_bytes()
#1974
Use chacha in get_secure_random_bytes()
#1974
Conversation
c896c0b
to
a512d3d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Github won't let me get there, but note that rand_bytes_unique_start
is still initialized but now unused:
a512d3d
to
d3f14e7
Compare
@tnull Thank you for your comments. Somehow I didn't notice the unused value warnings. In d3f14e7 I also removed I'm still missing fixing the failing test |
#1984 should fix it :) |
lightning/src/chain/keysinterface.rs
Outdated
let mut nonce = Vec::new(); | ||
nonce.append(&mut starting_time_secs.to_be_bytes().to_vec()); | ||
nonce.append(&mut starting_time_nanos.to_be_bytes().to_vec()); | ||
let chacha = Mutex::new(ChaCha20::new(seed, &nonce)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can avoid three vec allocations by having a fixed array and copying the bytes into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I've changed to that approach in fac7b4e.
d943b28
to
17860ac
Compare
lightning/src/chain/keysinterface.rs
Outdated
rand_bytes_unique_start.input(&starting_time_nanos.to_be_bytes()); | ||
rand_bytes_unique_start.input(seed); | ||
let mut rand_bytes_unique_start = [0u8; 32]; | ||
rand_bytes_unique_start[..8].copy_from_slice(&starting_time_secs.to_be_bytes()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to hash seed
in here somehow. IMO we should keep it as a hash, basically the way it was and just complete the hash rather than leaving it as an unfinished engine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, was thinking we may just want to restore and use the original rand_bytes_unique_start
as the ChaCha seed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, restore the old code but complete the hash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, if I understood you correctly, this is what you propose -> 6472af6
lightning/src/chain/keysinterface.rs
Outdated
|
||
for _ in 1..5 { | ||
let keys_manager_clone = Arc::clone(&keys_manager); | ||
thread::spawn(move || { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have to join these threads at the end, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, ty! Applied in 34fa5af
lightning/src/chain/keysinterface.rs
Outdated
rand_bytes_child_index: AtomicUsize, | ||
rand_bytes_unique_start: Sha256State, | ||
rand_bytes_unique_start: [u8; 32], | ||
rand_bytes_index: AtomicUsize, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be a util::atomic_counter::AtomicCounter
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied in 6472af6
17860ac
to
6472af6
Compare
6472af6
to
d08d4f8
Compare
LGTM, will let tnull take a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, repeated the benchmarks locally, which drew a pretty clear picture:
| Threads | Method | Time (ns) |
|---------+----------------+-----------|
| 1 | Hashing | 1,805,139 |
| 1 | ChaCha/Mutex | 6,308 |
| 1 | ChaCha/Counter | 10,725 |
| 3 | Hashing | 1,875,495 |
| 3 | ChaCha/Mutex | 144,687 |
| 3 | ChaCha/Counter | 18,778 |
| 5 | Hashing | 1,898,584 |
| 5 | ChaCha/Mutex | 92,116 |
| 5 | ChaCha/Counter | 40,465 |
|---------+----------------+-----------|
I'd say if we're confident in the counter method, it is probably the way to go.
That said, I'd also be happy to with the Mutex
variant, as it's more straight forward, the performance difference is really not that big and realistically the kind of lock contention simulated in the benchmark is really unlikely to happen in production. Note however that in this case we may want to throw in a Sha256
for good measure, instead of using the seed
directly.
lightning/src/chain/keysinterface.rs
Outdated
nonce[..8].copy_from_slice(&starting_time_secs.to_be_bytes()); | ||
nonce[8..12].copy_from_slice(&starting_time_nanos.to_be_bytes()); | ||
let chacha = Mutex::new(ChaCha20::new(seed, &nonce)); | ||
let mut rand_bytes_unique_start = Sha256::engine(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Rather than shadowing the variable, can we maybe rename this rand_engine_unique_start
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, applied in 1456770.
I'm more comfortable using the counter method - the |
Please also rebase/squash the fixup commit at the end into the appropriate commit. |
d08d4f8
to
1456770
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I think I'm ACK after the tests pass.
1456770
to
f19821d
Compare
Addresses #1958