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
| @@ -1143,7 +1143,7 @@ impl KeysInterface for KeysManager { | |||
| fn ecdh(&self, recipient: Recipient, other_key: &PublicKey, tweak: Option<&[u8; 32]>) -> Result<SharedSecret, ()> { | |||
| 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
| @@ -265,7 +265,7 @@ pub fn derive_private_key<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>, per_co | |||
| let res = Sha256::from_engine(sha).into_inner(); | |||
|
|
|||
| 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
| @@ -196,7 +196,7 @@ impl<Signer: Sign, K: Deref, L: Deref> OnionMessenger<Signer, K, L> | |||
| Hmac::from_engine(hmac).into_inner() | |||
| }; | |||
| 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 |
No description provided.