Update bitcoin crate to 0.29.0 #1658
Conversation
devrandom
force-pushed
the
2022-08-bitcoin-0-29
branch
from
345e69e to
1084020
Compare
lightning/src/chain/keysinterface.rs
Outdated
| let mut node_secret = self.get_node_secret(recipient)?; | ||
| if let Some(tweak) = tweak { | ||
| node_secret.mul_assign(tweak).map_err(|_| ())?; | ||
| node_secret = node_secret.mul_tweak(&Scalar::from_be_bytes(tweak.clone()).unwrap()).map_err(|_| ())?; |
There was a problem hiding this comment.
not sure if there's a way to eliminate this clone op
There was a problem hiding this comment.
[u8; 32] is Copy, so Scalar::from_be_bytes(*tweak).unwrap() should work well. Even better, you could change the function signature to accept Option<&Scalar> instead and move unwrap to the caller.
There was a problem hiding this comment.
ah, yes of course. will do the former for now and look at the cleanup a bit later
There was a problem hiding this comment.
Out of curiosity I looked at how the code changes in practice. I'm pretty happy how it turned out. Just two tips:
Consider removing intermediary mut variables when tweaking the keys, the intention is to make the code clearer because the value is semantically different after tweaking. So something like let new_descriptive_name = old_decriptive_name.mul_tweak(tweak);
Second, consider propagating types to your structures so that you clarify semantics in your own code and move conversions to more appropriate places.
Also, FYI the release got a bit messed up.
lightning/src/ln/chan_utils.rs
Outdated
|
|
||
| let mut key = base_secret.clone(); | ||
| key.add_assign(&res)?; | ||
| key = key.add_tweak(&Scalar::from_be_bytes(res).unwrap())?; |
There was a problem hiding this comment.
You could get rid of mut and an extra line if you write let key = base_secret.add_tweak(&Scalar::from_be_bytes(res).unwrap())?;
There was a problem hiding this comment.
it ended up that many of the intermediate variables were superfluous anyway
devrandom
force-pushed
the
2022-08-bitcoin-0-29
branch
3 times, most recently
from
97a5df0 to
ea67c70
Compare
valentinewallace
left a comment
valentinewallace
left a comment
There was a problem hiding this comment.
Wondering about the unwraps but otherwise ACK
| }; | ||
| match self.keys_manager.ecdh(Recipient::Node, &msg.onion_routing_packet.public_key, | ||
| Some(&blinding_factor)) | ||
| Some(&Scalar::from_be_bytes(blinding_factor).unwrap())) |
There was a problem hiding this comment.
Should we be mapping the err here instead of unwraping? Perhaps it's guaranteed to succeed anyway.. Similar in chan_utils and onion_utils
There was a problem hiding this comment.
I think unwrap'ing, a scalar conversion from a hash is safe, same as unwrap'ing a secretkey conversion from a hash.
There was a problem hiding this comment.
(because the chance of failure is 2 ** -128, so more likely you have a bug or hardware failure)
|
Needs squash, at least. |
devrandom
force-pushed
the
2022-08-bitcoin-0-29
branch
from
074c09b to
11166aa
Compare
|
squashed into two commits - let me know if you prefer one |
4 participants
No description provided.