Simplify + clarify random-bytes-fetching from KeysInterface #674
Conversation
Codecov Report
@@ Coverage Diff @@
## master #674 +/- ##
==========================================
+ Coverage 91.36% 91.45% +0.09%
==========================================
Files 35 35
Lines 21703 22198 +495
==========================================
+ Hits 19828 20302 +474
- Misses 1875 1896 +21
Continue to review full report at Codecov.
|
| /// transaction is created, at which point they will use the outpoint in the funding | ||
| /// transaction. | ||
| fn get_channel_id(&self) -> [u8; 32]; | ||
| /// Gets a unique, cryptographically-secure, random 32 byte value. This is used for encrypting |
There was a problem hiding this comment.
Is this an API requirement change to switch from using a PRNG to CSPRNG ? This is better, but if so rename prng_seed mentions.
There was a problem hiding this comment.
I only see one doc comment mentioning RNG (which now says CSRNG).
| let child_ix = self.channel_id_child_index.fetch_add(1, Ordering::AcqRel); | ||
| let child_privkey = self.channel_id_master_key.ckd_priv(&self.secp_ctx, ChildNumber::from_hardened_idx(child_ix as u32).expect("key space exhausted")).expect("Your RNG is busted"); | ||
| let child_ix = self.rand_bytes_child_index.fetch_add(1, Ordering::AcqRel); | ||
| let child_privkey = self.rand_bytes_master_key.ckd_priv(&self.secp_ctx, ChildNumber::from_hardened_idx(child_ix as u32).expect("key space exhausted")).expect("Your RNG is busted"); |
There was a problem hiding this comment.
We have "RNG is busted" in a ton of places. I don't think its really all that critical to explicitly say CSRNG in a panic message (which even a biased RNG should suffice to not hit), as long as the docs (which hopefully is the only thing users ever see) are good.
| let override_channel_id = self.override_channel_id_priv.lock().unwrap(); | ||
| let override_session_key = self.override_session_priv.lock().unwrap(); | ||
| if override_channel_id.is_some() && override_session_key.is_some() { | ||
| panic!("We don't know which override key to use!"); |
There was a problem hiding this comment.
By your commit message, you mean the reason to have both functions to the interface was to potentially override both keys in test framework, which we never need to do so actually ?
There was a problem hiding this comment.
Pretty much, yes, but it was also just a historical quirk - we had different ways to get data into the KeyInterface and the easiest way to keep that the way it was was to have multiple override hooks.
Due to a desire to be able to override temporary channel IDs and onion keys, KeysInterface had two separate fetch-random-32-bytes interfaces - an onion-key specific version which fetched 2 random 32 byte strings and a temporary-channel-id specific version. It turns out, we never actually need to override both at once (as creating a new channel and sending an outbound payment are always separate top-level calls), so there's no reason to add two functions to the interface when both really do the same thing.
TheBlueMatt
force-pushed
the
2020-08-keyif-rand-names
branch
from
cad08d1 to
6497465
Compare
|
Code Review ACK 6497465 |
Due to a desire to be able to override temporary channel IDs and
onion keys, KeysInterface had two separate fetch-random-32-bytes
interfaces - an onion-key specific version which fetched 2 random
32 byte strings and a temporary-channel-id specific version.
It turns out, we never actually need to override both at once (as
creating a new channel and sending an outbound payment are always
separate top-level calls), so there's no reason to add two
functions to the interface when both really do the same thing.