From 27461902ab3acd1b665ec9a91fd1768dbfee1c36 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 30 Nov 2022 22:21:24 +0000 Subject: [PATCH 1/3] Remove unreachable `Err` cases on `derive_{public,private}_key` The `derive_{public,private}_key` methods hash the two input keys and then add them to the input public key. Because addition can fail if the tweak is the inverse of the secret key this method currently returns a `Result`. However, it is not cryptographically possible to reach the error case - in order to create an issue, the SHA-256 hash of the `base_point` (and other data) must be the inverse of the `base_point`('s secret key). Because changing the `base_point` changes the hash in an unpredictable way, there should be no way to construct such a `base_point`. --- lightning/src/chain/channelmonitor.rs | 17 +++++--------- lightning/src/chain/keysinterface.rs | 33 ++++++++++++--------------- lightning/src/ln/chan_utils.rs | 23 ++++++++----------- lightning/src/ln/channel.rs | 4 ++-- 4 files changed, 32 insertions(+), 45 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 92f1a8df4..a93985e09 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -2444,7 +2444,7 @@ impl ChannelMonitorImpl { let per_commitment_key = ignore_error!(SecretKey::from_slice(&secret)); let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key); let revocation_pubkey = ignore_error!(chan_utils::derive_public_revocation_key(&self.secp_ctx, &per_commitment_point, &self.holder_revocation_basepoint)); - let delayed_key = ignore_error!(chan_utils::derive_public_key(&self.secp_ctx, &PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key), &self.counterparty_commitment_params.counterparty_delayed_payment_base_key)); + let delayed_key = chan_utils::derive_public_key(&self.secp_ctx, &PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key), &self.counterparty_commitment_params.counterparty_delayed_payment_base_key); let revokeable_redeemscript = chan_utils::get_revokeable_redeemscript(&revocation_pubkey, self.counterparty_commitment_params.on_counterparty_tx_csv, &delayed_key); let revokeable_p2wsh = revokeable_redeemscript.to_v0_p2wsh(); @@ -2560,17 +2560,12 @@ impl ChannelMonitorImpl { if let Ok(revocation_pubkey) = chan_utils::derive_public_revocation_key( &self.secp_ctx, &per_commitment_point, &self.holder_revocation_basepoint) { - if let Ok(delayed_key) = chan_utils::derive_public_key(&self.secp_ctx, + let delayed_key = chan_utils::derive_public_key(&self.secp_ctx, &per_commitment_point, - &self.counterparty_commitment_params.counterparty_delayed_payment_base_key) - { - Some(chan_utils::get_revokeable_redeemscript(&revocation_pubkey, - self.counterparty_commitment_params.on_counterparty_tx_csv, - &delayed_key).to_v0_p2wsh()) - } else { - debug_assert!(false, "Failed to derive a delayed payment key for a commitment state we accepted"); - None - } + &self.counterparty_commitment_params.counterparty_delayed_payment_base_key); + Some(chan_utils::get_revokeable_redeemscript(&revocation_pubkey, + self.counterparty_commitment_params.on_counterparty_tx_csv, + &delayed_key).to_v0_p2wsh()) } else { debug_assert!(false, "Failed to derive a revocation pubkey key for a commitment state we accepted"); None diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs index 9f8197690..266544d78 100644 --- a/lightning/src/chain/keysinterface.rs +++ b/lightning/src/chain/keysinterface.rs @@ -655,8 +655,7 @@ impl InMemorySigner { if spend_tx.input[input_idx].previous_output != descriptor.outpoint.into_bitcoin_outpoint() { return Err(()); } if spend_tx.input[input_idx].sequence.0 != descriptor.to_self_delay as u32 { return Err(()); } - let delayed_payment_key = chan_utils::derive_private_key(&secp_ctx, &descriptor.per_commitment_point, &self.delayed_payment_base_key) - .expect("We constructed the payment_base_key, so we can only fail here if the RNG is busted."); + let delayed_payment_key = chan_utils::derive_private_key(&secp_ctx, &descriptor.per_commitment_point, &self.delayed_payment_base_key); let delayed_payment_pubkey = PublicKey::from_secret_key(&secp_ctx, &delayed_payment_key); let witness_script = chan_utils::get_revokeable_redeemscript(&descriptor.revocation_pubkey, descriptor.to_self_delay, &delayed_payment_pubkey); let sighash = hash_to_message!(&sighash::SighashCache::new(spend_tx).segwit_signature_hash(input_idx, &witness_script, descriptor.output.value, EcdsaSighashType::All).unwrap()[..]); @@ -710,7 +709,7 @@ impl BaseSign for InMemorySigner { let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, self.opt_anchors(), &keys); let htlc_sighashtype = if self.opt_anchors() { EcdsaSighashType::SinglePlusAnyoneCanPay } else { EcdsaSighashType::All }; let htlc_sighash = hash_to_message!(&sighash::SighashCache::new(&htlc_tx).segwit_signature_hash(0, &htlc_redeemscript, htlc.amount_msat / 1000, htlc_sighashtype).unwrap()[..]); - let holder_htlc_key = chan_utils::derive_private_key(&secp_ctx, &keys.per_commitment_point, &self.htlc_base_key).map_err(|_| ())?; + let holder_htlc_key = chan_utils::derive_private_key(&secp_ctx, &keys.per_commitment_point, &self.htlc_base_key); htlc_sigs.push(sign(secp_ctx, &htlc_sighash, &holder_htlc_key)); } @@ -747,7 +746,7 @@ impl BaseSign for InMemorySigner { let per_commitment_point = PublicKey::from_secret_key(secp_ctx, &per_commitment_key); let revocation_pubkey = chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &self.pubkeys().revocation_basepoint).map_err(|_| ())?; let witness_script = { - let counterparty_delayedpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.counterparty_pubkeys().delayed_payment_basepoint).map_err(|_| ())?; + let counterparty_delayedpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.counterparty_pubkeys().delayed_payment_basepoint); chan_utils::get_revokeable_redeemscript(&revocation_pubkey, self.holder_selected_contest_delay(), &counterparty_delayedpubkey) }; let mut sighash_parts = sighash::SighashCache::new(justice_tx); @@ -760,8 +759,8 @@ impl BaseSign for InMemorySigner { let per_commitment_point = PublicKey::from_secret_key(secp_ctx, &per_commitment_key); let revocation_pubkey = chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &self.pubkeys().revocation_basepoint).map_err(|_| ())?; let witness_script = { - let counterparty_htlcpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.counterparty_pubkeys().htlc_basepoint).map_err(|_| ())?; - let holder_htlcpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.pubkeys().htlc_basepoint).map_err(|_| ())?; + let counterparty_htlcpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.counterparty_pubkeys().htlc_basepoint); + let holder_htlcpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.pubkeys().htlc_basepoint); chan_utils::get_htlc_redeemscript_with_explicit_keys(&htlc, self.opt_anchors(), &counterparty_htlcpubkey, &holder_htlcpubkey, &revocation_pubkey) }; let mut sighash_parts = sighash::SighashCache::new(justice_tx); @@ -770,19 +769,15 @@ impl BaseSign for InMemorySigner { } fn sign_counterparty_htlc_transaction(&self, htlc_tx: &Transaction, input: usize, amount: u64, per_commitment_point: &PublicKey, htlc: &HTLCOutputInCommitment, secp_ctx: &Secp256k1) -> Result { - if let Ok(htlc_key) = chan_utils::derive_private_key(&secp_ctx, &per_commitment_point, &self.htlc_base_key) { - let witness_script = if let Ok(revocation_pubkey) = chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &self.pubkeys().revocation_basepoint) { - if let Ok(counterparty_htlcpubkey) = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.counterparty_pubkeys().htlc_basepoint) { - if let Ok(htlcpubkey) = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.pubkeys().htlc_basepoint) { - chan_utils::get_htlc_redeemscript_with_explicit_keys(&htlc, self.opt_anchors(), &counterparty_htlcpubkey, &htlcpubkey, &revocation_pubkey) - } else { return Err(()) } - } else { return Err(()) } - } else { return Err(()) }; - let mut sighash_parts = sighash::SighashCache::new(htlc_tx); - let sighash = hash_to_message!(&sighash_parts.segwit_signature_hash(input, &witness_script, amount, EcdsaSighashType::All).unwrap()[..]); - return Ok(sign(secp_ctx, &sighash, &htlc_key)) - } - Err(()) + let htlc_key = chan_utils::derive_private_key(&secp_ctx, &per_commitment_point, &self.htlc_base_key); + let witness_script = if let Ok(revocation_pubkey) = chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &self.pubkeys().revocation_basepoint) { + let counterparty_htlcpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.counterparty_pubkeys().htlc_basepoint); + let htlcpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.pubkeys().htlc_basepoint); + chan_utils::get_htlc_redeemscript_with_explicit_keys(&htlc, self.opt_anchors(), &counterparty_htlcpubkey, &htlcpubkey, &revocation_pubkey) + } else { return Err(()) }; + let mut sighash_parts = sighash::SighashCache::new(htlc_tx); + let sighash = hash_to_message!(&sighash_parts.segwit_signature_hash(input, &witness_script, amount, EcdsaSighashType::All).unwrap()[..]); + Ok(sign(secp_ctx, &sighash, &htlc_key)) } fn sign_closing_transaction(&self, closing_tx: &ClosingTransaction, secp_ctx: &Secp256k1) -> Result { diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index 3a6e81e63..0dbf18684 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -316,32 +316,29 @@ impl Readable for CounterpartyCommitmentSecrets { /// Derives a per-commitment-transaction private key (eg an htlc key or delayed_payment key) /// from the base secret and the per_commitment_point. -/// -/// Note that this is infallible iff we trust that at least one of the two input keys are randomly -/// generated (ie our own). -pub fn derive_private_key(secp_ctx: &Secp256k1, per_commitment_point: &PublicKey, base_secret: &SecretKey) -> Result { +pub fn derive_private_key(secp_ctx: &Secp256k1, per_commitment_point: &PublicKey, base_secret: &SecretKey) -> SecretKey { let mut sha = Sha256::engine(); sha.input(&per_commitment_point.serialize()); sha.input(&PublicKey::from_secret_key(&secp_ctx, &base_secret).serialize()); let res = Sha256::from_engine(sha).into_inner(); base_secret.clone().add_tweak(&Scalar::from_be_bytes(res).unwrap()) + .expect("Addition only fails if the tweak is the inverse of the key. This is not possible when the tweak contains the hash of the key.") } /// Derives a per-commitment-transaction public key (eg an htlc key or a delayed_payment key) /// from the base point and the per_commitment_key. This is the public equivalent of /// derive_private_key - using only public keys to derive a public key instead of private keys. -/// -/// Note that this is infallible iff we trust that at least one of the two input keys are randomly -/// generated (ie our own). -pub fn derive_public_key(secp_ctx: &Secp256k1, per_commitment_point: &PublicKey, base_point: &PublicKey) -> Result { +pub fn derive_public_key(secp_ctx: &Secp256k1, per_commitment_point: &PublicKey, base_point: &PublicKey) -> PublicKey { let mut sha = Sha256::engine(); sha.input(&per_commitment_point.serialize()); sha.input(&base_point.serialize()); let res = Sha256::from_engine(sha).into_inner(); - let hashkey = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&res)?); + let hashkey = PublicKey::from_secret_key(&secp_ctx, + &SecretKey::from_slice(&res).expect("Hashes should always be valid keys unless SHA-256 is broken")); base_point.combine(&hashkey) + .expect("Addition only fails if the tweak is the inverse of the key. This is not possible when the tweak contains the hash of the key.") } /// Derives a per-commitment-transaction revocation key from its constituent parts. @@ -483,9 +480,9 @@ impl TxCreationKeys { Ok(TxCreationKeys { per_commitment_point: per_commitment_point.clone(), revocation_key: derive_public_revocation_key(&secp_ctx, &per_commitment_point, &countersignatory_revocation_base)?, - broadcaster_htlc_key: derive_public_key(&secp_ctx, &per_commitment_point, &broadcaster_htlc_base)?, - countersignatory_htlc_key: derive_public_key(&secp_ctx, &per_commitment_point, &countersignatory_htlc_base)?, - broadcaster_delayed_payment_key: derive_public_key(&secp_ctx, &per_commitment_point, &broadcaster_delayed_payment_base)?, + broadcaster_htlc_key: derive_public_key(&secp_ctx, &per_commitment_point, &broadcaster_htlc_base), + countersignatory_htlc_key: derive_public_key(&secp_ctx, &per_commitment_point, &countersignatory_htlc_base), + broadcaster_delayed_payment_key: derive_public_key(&secp_ctx, &per_commitment_point, &broadcaster_delayed_payment_base), }) } @@ -1506,7 +1503,7 @@ impl<'a> TrustedCommitmentTransaction<'a> { let keys = &inner.keys; let txid = inner.built.txid; let mut ret = Vec::with_capacity(inner.htlcs.len()); - let holder_htlc_key = derive_private_key(secp_ctx, &inner.keys.per_commitment_point, htlc_base_key).map_err(|_| ())?; + let holder_htlc_key = derive_private_key(secp_ctx, &inner.keys.per_commitment_point, htlc_base_key); for this_htlc in inner.htlcs.iter() { assert!(this_htlc.transaction_output_index.is_some()); diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 5cc4f4a36..0c0cbe83f 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -7962,10 +7962,10 @@ mod tests { let per_commitment_point = PublicKey::from_secret_key(&secp_ctx, &per_commitment_secret); assert_eq!(per_commitment_point.serialize()[..], hex::decode("025f7117a78150fe2ef97db7cfc83bd57b2e2c0d0dd25eaf467a4a1c2a45ce1486").unwrap()[..]); - assert_eq!(chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &base_point).unwrap().serialize()[..], + assert_eq!(chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &base_point).serialize()[..], hex::decode("0235f2dbfaa89b57ec7b055afe29849ef7ddfeb1cefdb9ebdc43f5494984db29e5").unwrap()[..]); - assert_eq!(chan_utils::derive_private_key(&secp_ctx, &per_commitment_point, &base_secret).unwrap(), + assert_eq!(chan_utils::derive_private_key(&secp_ctx, &per_commitment_point, &base_secret), SecretKey::from_slice(&hex::decode("cbced912d3b21bf196a766651e436aff192362621ce317704ea2f75d87e7be0f").unwrap()[..]).unwrap()); assert_eq!(chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &base_point).unwrap().serialize()[..], From 5671d2930d996e864614703696ed2cba5e47e4f0 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 30 Nov 2022 22:34:11 +0000 Subject: [PATCH 2/3] Remove unreachable `Err` cases on `derive_*_revocation_key` The `derive_{public,private}_revocation_key` methods hash the two input keys and then multiply the two input keys by hashed values before adding them together. Because addition can fail if the tweak is the inverse of the secret key this method currently returns a `Result`. However, it is not cryptographically possible to reach the error case - in order to create an issue, the point-multiplied-by-hash values must be the inverse of each other, however each point commits the SHA-256 hash of both keys together. Thus, because changing either key changes the hashes (and the ultimate points added together) in an unpredictable way, there should be no way to construct such points. --- lightning/src/chain/channelmonitor.rs | 34 ++++++++++----------------- lightning/src/chain/keysinterface.rs | 17 +++++++------- lightning/src/ln/chan_utils.rs | 27 +++++++++++++-------- lightning/src/ln/channel.rs | 4 ++-- 4 files changed, 40 insertions(+), 42 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index a93985e09..cc88a0119 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -2443,7 +2443,7 @@ impl ChannelMonitorImpl { let secret = self.get_secret(commitment_number).unwrap(); let per_commitment_key = ignore_error!(SecretKey::from_slice(&secret)); let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key); - let revocation_pubkey = ignore_error!(chan_utils::derive_public_revocation_key(&self.secp_ctx, &per_commitment_point, &self.holder_revocation_basepoint)); + let revocation_pubkey = chan_utils::derive_public_revocation_key(&self.secp_ctx, &per_commitment_point, &self.holder_revocation_basepoint); let delayed_key = chan_utils::derive_public_key(&self.secp_ctx, &PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key), &self.counterparty_commitment_params.counterparty_delayed_payment_base_key); let revokeable_redeemscript = chan_utils::get_revokeable_redeemscript(&revocation_pubkey, self.counterparty_commitment_params.on_counterparty_tx_csv, &delayed_key); @@ -2556,26 +2556,18 @@ impl ChannelMonitorImpl { } else { return (claimable_outpoints, to_counterparty_output_info); }; if let Some(transaction) = tx { - let revokeable_p2wsh_opt = - if let Ok(revocation_pubkey) = chan_utils::derive_public_revocation_key( - &self.secp_ctx, &per_commitment_point, &self.holder_revocation_basepoint) - { - let delayed_key = chan_utils::derive_public_key(&self.secp_ctx, - &per_commitment_point, - &self.counterparty_commitment_params.counterparty_delayed_payment_base_key); - Some(chan_utils::get_revokeable_redeemscript(&revocation_pubkey, - self.counterparty_commitment_params.on_counterparty_tx_csv, - &delayed_key).to_v0_p2wsh()) - } else { - debug_assert!(false, "Failed to derive a revocation pubkey key for a commitment state we accepted"); - None - }; - if let Some(revokeable_p2wsh) = revokeable_p2wsh_opt { - for (idx, outp) in transaction.output.iter().enumerate() { - if outp.script_pubkey == revokeable_p2wsh { - to_counterparty_output_info = - Some((idx.try_into().expect("Can't have > 2^32 outputs"), outp.value)); - } + let revocation_pubkey = chan_utils::derive_public_revocation_key( + &self.secp_ctx, &per_commitment_point, &self.holder_revocation_basepoint); + let delayed_key = chan_utils::derive_public_key(&self.secp_ctx, + &per_commitment_point, + &self.counterparty_commitment_params.counterparty_delayed_payment_base_key); + let revokeable_p2wsh = chan_utils::get_revokeable_redeemscript(&revocation_pubkey, + self.counterparty_commitment_params.on_counterparty_tx_csv, + &delayed_key).to_v0_p2wsh(); + for (idx, outp) in transaction.output.iter().enumerate() { + if outp.script_pubkey == revokeable_p2wsh { + to_counterparty_output_info = + Some((idx.try_into().expect("Can't have > 2^32 outputs"), outp.value)); } } } diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs index 266544d78..db1c6e943 100644 --- a/lightning/src/chain/keysinterface.rs +++ b/lightning/src/chain/keysinterface.rs @@ -742,9 +742,9 @@ impl BaseSign for InMemorySigner { } fn sign_justice_revoked_output(&self, justice_tx: &Transaction, input: usize, amount: u64, per_commitment_key: &SecretKey, secp_ctx: &Secp256k1) -> Result { - let revocation_key = chan_utils::derive_private_revocation_key(&secp_ctx, &per_commitment_key, &self.revocation_base_key).map_err(|_| ())?; + let revocation_key = chan_utils::derive_private_revocation_key(&secp_ctx, &per_commitment_key, &self.revocation_base_key); let per_commitment_point = PublicKey::from_secret_key(secp_ctx, &per_commitment_key); - let revocation_pubkey = chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &self.pubkeys().revocation_basepoint).map_err(|_| ())?; + let revocation_pubkey = chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &self.pubkeys().revocation_basepoint); let witness_script = { let counterparty_delayedpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.counterparty_pubkeys().delayed_payment_basepoint); chan_utils::get_revokeable_redeemscript(&revocation_pubkey, self.holder_selected_contest_delay(), &counterparty_delayedpubkey) @@ -755,9 +755,9 @@ impl BaseSign for InMemorySigner { } fn sign_justice_revoked_htlc(&self, justice_tx: &Transaction, input: usize, amount: u64, per_commitment_key: &SecretKey, htlc: &HTLCOutputInCommitment, secp_ctx: &Secp256k1) -> Result { - let revocation_key = chan_utils::derive_private_revocation_key(&secp_ctx, &per_commitment_key, &self.revocation_base_key).map_err(|_| ())?; + let revocation_key = chan_utils::derive_private_revocation_key(&secp_ctx, &per_commitment_key, &self.revocation_base_key); let per_commitment_point = PublicKey::from_secret_key(secp_ctx, &per_commitment_key); - let revocation_pubkey = chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &self.pubkeys().revocation_basepoint).map_err(|_| ())?; + let revocation_pubkey = chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &self.pubkeys().revocation_basepoint); let witness_script = { let counterparty_htlcpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.counterparty_pubkeys().htlc_basepoint); let holder_htlcpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.pubkeys().htlc_basepoint); @@ -770,11 +770,10 @@ impl BaseSign for InMemorySigner { fn sign_counterparty_htlc_transaction(&self, htlc_tx: &Transaction, input: usize, amount: u64, per_commitment_point: &PublicKey, htlc: &HTLCOutputInCommitment, secp_ctx: &Secp256k1) -> Result { let htlc_key = chan_utils::derive_private_key(&secp_ctx, &per_commitment_point, &self.htlc_base_key); - let witness_script = if let Ok(revocation_pubkey) = chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &self.pubkeys().revocation_basepoint) { - let counterparty_htlcpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.counterparty_pubkeys().htlc_basepoint); - let htlcpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.pubkeys().htlc_basepoint); - chan_utils::get_htlc_redeemscript_with_explicit_keys(&htlc, self.opt_anchors(), &counterparty_htlcpubkey, &htlcpubkey, &revocation_pubkey) - } else { return Err(()) }; + let revocation_pubkey = chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &self.pubkeys().revocation_basepoint); + let counterparty_htlcpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.counterparty_pubkeys().htlc_basepoint); + let htlcpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.pubkeys().htlc_basepoint); + let witness_script = chan_utils::get_htlc_redeemscript_with_explicit_keys(&htlc, self.opt_anchors(), &counterparty_htlcpubkey, &htlcpubkey, &revocation_pubkey); let mut sighash_parts = sighash::SighashCache::new(htlc_tx); let sighash = hash_to_message!(&sighash_parts.segwit_signature_hash(input, &witness_script, amount, EcdsaSighashType::All).unwrap()[..]); Ok(sign(secp_ctx, &sighash, &htlc_key)) diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index 0dbf18684..0b9381d2d 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -347,10 +347,9 @@ pub fn derive_public_key(secp_ctx: &Secp256k1, per_com /// commitment transaction, thus per_commitment_secret always come from cheater /// and revocation_base_secret always come from punisher, which is the broadcaster /// of the transaction spending with this key knowledge. -/// -/// Note that this is infallible iff we trust that at least one of the two input keys are randomly -/// generated (ie our own). -pub fn derive_private_revocation_key(secp_ctx: &Secp256k1, per_commitment_secret: &SecretKey, countersignatory_revocation_base_secret: &SecretKey) -> Result { +pub fn derive_private_revocation_key(secp_ctx: &Secp256k1, + per_commitment_secret: &SecretKey, countersignatory_revocation_base_secret: &SecretKey) +-> SecretKey { let countersignatory_revocation_base_point = PublicKey::from_secret_key(&secp_ctx, &countersignatory_revocation_base_secret); let per_commitment_point = PublicKey::from_secret_key(&secp_ctx, &per_commitment_secret); @@ -369,9 +368,12 @@ pub fn derive_private_revocation_key(secp_ctx: &Secp256k1 Sha256::from_engine(sha).into_inner() }; - let countersignatory_contrib = countersignatory_revocation_base_secret.clone().mul_tweak(&Scalar::from_be_bytes(rev_append_commit_hash_key).unwrap())?; - let broadcaster_contrib = per_commitment_secret.clone().mul_tweak(&Scalar::from_be_bytes(commit_append_rev_hash_key).unwrap())?; + let countersignatory_contrib = countersignatory_revocation_base_secret.clone().mul_tweak(&Scalar::from_be_bytes(rev_append_commit_hash_key).unwrap()) + .expect("Multiplying a secret key by a hash is expected to never fail per secp256k1 docs"); + let broadcaster_contrib = per_commitment_secret.clone().mul_tweak(&Scalar::from_be_bytes(commit_append_rev_hash_key).unwrap()) + .expect("Multiplying a secret key by a hash is expected to never fail per secp256k1 docs"); countersignatory_contrib.add_tweak(&Scalar::from_be_bytes(broadcaster_contrib.secret_bytes()).unwrap()) + .expect("Addition only fails if the tweak is the inverse of the key. This is not possible when the tweak commits to the key.") } /// Derives a per-commitment-transaction revocation public key from its constituent parts. This is @@ -385,7 +387,9 @@ pub fn derive_private_revocation_key(secp_ctx: &Secp256k1 /// /// Note that this is infallible iff we trust that at least one of the two input keys are randomly /// generated (ie our own). -pub fn derive_public_revocation_key(secp_ctx: &Secp256k1, per_commitment_point: &PublicKey, countersignatory_revocation_base_point: &PublicKey) -> Result { +pub fn derive_public_revocation_key(secp_ctx: &Secp256k1, + per_commitment_point: &PublicKey, countersignatory_revocation_base_point: &PublicKey) +-> PublicKey { let rev_append_commit_hash_key = { let mut sha = Sha256::engine(); sha.input(&countersignatory_revocation_base_point.serialize()); @@ -401,9 +405,12 @@ pub fn derive_public_revocation_key(secp_ctx: &Secp2 Sha256::from_engine(sha).into_inner() }; - let countersignatory_contrib = countersignatory_revocation_base_point.clone().mul_tweak(&secp_ctx, &Scalar::from_be_bytes(rev_append_commit_hash_key).unwrap())?; - let broadcaster_contrib = per_commitment_point.clone().mul_tweak(&secp_ctx, &Scalar::from_be_bytes(commit_append_rev_hash_key).unwrap())?; + let countersignatory_contrib = countersignatory_revocation_base_point.clone().mul_tweak(&secp_ctx, &Scalar::from_be_bytes(rev_append_commit_hash_key).unwrap()) + .expect("Multiplying a valid public key by a hash is expected to never fail per secp256k1 docs"); + let broadcaster_contrib = per_commitment_point.clone().mul_tweak(&secp_ctx, &Scalar::from_be_bytes(commit_append_rev_hash_key).unwrap()) + .expect("Multiplying a valid public key by a hash is expected to never fail per secp256k1 docs"); countersignatory_contrib.combine(&broadcaster_contrib) + .expect("Addition only fails if the tweak is the inverse of the key. This is not possible when the tweak commits to the key.") } /// The set of public keys which are used in the creation of one commitment transaction. @@ -479,7 +486,7 @@ impl TxCreationKeys { pub fn derive_new(secp_ctx: &Secp256k1, per_commitment_point: &PublicKey, broadcaster_delayed_payment_base: &PublicKey, broadcaster_htlc_base: &PublicKey, countersignatory_revocation_base: &PublicKey, countersignatory_htlc_base: &PublicKey) -> Result { Ok(TxCreationKeys { per_commitment_point: per_commitment_point.clone(), - revocation_key: derive_public_revocation_key(&secp_ctx, &per_commitment_point, &countersignatory_revocation_base)?, + revocation_key: derive_public_revocation_key(&secp_ctx, &per_commitment_point, &countersignatory_revocation_base), broadcaster_htlc_key: derive_public_key(&secp_ctx, &per_commitment_point, &broadcaster_htlc_base), countersignatory_htlc_key: derive_public_key(&secp_ctx, &per_commitment_point, &countersignatory_htlc_base), broadcaster_delayed_payment_key: derive_public_key(&secp_ctx, &per_commitment_point, &broadcaster_delayed_payment_base), diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 0c0cbe83f..ef7c77f0b 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -7968,10 +7968,10 @@ mod tests { assert_eq!(chan_utils::derive_private_key(&secp_ctx, &per_commitment_point, &base_secret), SecretKey::from_slice(&hex::decode("cbced912d3b21bf196a766651e436aff192362621ce317704ea2f75d87e7be0f").unwrap()[..]).unwrap()); - assert_eq!(chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &base_point).unwrap().serialize()[..], + assert_eq!(chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &base_point).serialize()[..], hex::decode("02916e326636d19c33f13e8c0c3a03dd157f332f3e99c317c141dd865eb01f8ff0").unwrap()[..]); - assert_eq!(chan_utils::derive_private_revocation_key(&secp_ctx, &per_commitment_secret, &base_secret).unwrap(), + assert_eq!(chan_utils::derive_private_revocation_key(&secp_ctx, &per_commitment_secret, &base_secret), SecretKey::from_slice(&hex::decode("d09ffff62ddb2297ab000cc85bcb4283fdeb6aa052affbc9dddcf33b61078110").unwrap()[..]).unwrap()); } From 2cfc1dbb44ef2b764f4f14abbc70d6757d4b12a4 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 30 Nov 2022 22:43:29 +0000 Subject: [PATCH 3/3] Remove unreachable `Err` cases when constructing `TxCreationKeys` --- lightning/src/chain/package.rs | 80 +++++++++++++--------------- lightning/src/ln/chan_utils.rs | 13 +++-- lightning/src/ln/channel.rs | 28 +++++----- lightning/src/ln/functional_tests.rs | 4 +- 4 files changed, 60 insertions(+), 65 deletions(-) diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index 1307ad0eb..cf92df295 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -381,57 +381,53 @@ impl PackageSolvingData { fn finalize_input(&self, bumped_tx: &mut Transaction, i: usize, onchain_handler: &mut OnchainTxHandler) -> bool { match self { PackageSolvingData::RevokedOutput(ref outp) => { - if let Ok(chan_keys) = TxCreationKeys::derive_new(&onchain_handler.secp_ctx, &outp.per_commitment_point, &outp.counterparty_delayed_payment_base_key, &outp.counterparty_htlc_base_key, &onchain_handler.signer.pubkeys().revocation_basepoint, &onchain_handler.signer.pubkeys().htlc_basepoint) { - let witness_script = chan_utils::get_revokeable_redeemscript(&chan_keys.revocation_key, outp.on_counterparty_tx_csv, &chan_keys.broadcaster_delayed_payment_key); - //TODO: should we panic on signer failure ? - if let Ok(sig) = onchain_handler.signer.sign_justice_revoked_output(&bumped_tx, i, outp.amount, &outp.per_commitment_key, &onchain_handler.secp_ctx) { - let mut ser_sig = sig.serialize_der().to_vec(); - ser_sig.push(EcdsaSighashType::All as u8); - bumped_tx.input[i].witness.push(ser_sig); - bumped_tx.input[i].witness.push(vec!(1)); - bumped_tx.input[i].witness.push(witness_script.clone().into_bytes()); - } else { return false; } - } + let chan_keys = TxCreationKeys::derive_new(&onchain_handler.secp_ctx, &outp.per_commitment_point, &outp.counterparty_delayed_payment_base_key, &outp.counterparty_htlc_base_key, &onchain_handler.signer.pubkeys().revocation_basepoint, &onchain_handler.signer.pubkeys().htlc_basepoint); + let witness_script = chan_utils::get_revokeable_redeemscript(&chan_keys.revocation_key, outp.on_counterparty_tx_csv, &chan_keys.broadcaster_delayed_payment_key); + //TODO: should we panic on signer failure ? + if let Ok(sig) = onchain_handler.signer.sign_justice_revoked_output(&bumped_tx, i, outp.amount, &outp.per_commitment_key, &onchain_handler.secp_ctx) { + let mut ser_sig = sig.serialize_der().to_vec(); + ser_sig.push(EcdsaSighashType::All as u8); + bumped_tx.input[i].witness.push(ser_sig); + bumped_tx.input[i].witness.push(vec!(1)); + bumped_tx.input[i].witness.push(witness_script.clone().into_bytes()); + } else { return false; } }, PackageSolvingData::RevokedHTLCOutput(ref outp) => { - if let Ok(chan_keys) = TxCreationKeys::derive_new(&onchain_handler.secp_ctx, &outp.per_commitment_point, &outp.counterparty_delayed_payment_base_key, &outp.counterparty_htlc_base_key, &onchain_handler.signer.pubkeys().revocation_basepoint, &onchain_handler.signer.pubkeys().htlc_basepoint) { - let witness_script = chan_utils::get_htlc_redeemscript_with_explicit_keys(&outp.htlc, onchain_handler.opt_anchors(), &chan_keys.broadcaster_htlc_key, &chan_keys.countersignatory_htlc_key, &chan_keys.revocation_key); - //TODO: should we panic on signer failure ? - if let Ok(sig) = onchain_handler.signer.sign_justice_revoked_htlc(&bumped_tx, i, outp.amount, &outp.per_commitment_key, &outp.htlc, &onchain_handler.secp_ctx) { - let mut ser_sig = sig.serialize_der().to_vec(); - ser_sig.push(EcdsaSighashType::All as u8); - bumped_tx.input[i].witness.push(ser_sig); - bumped_tx.input[i].witness.push(chan_keys.revocation_key.clone().serialize().to_vec()); - bumped_tx.input[i].witness.push(witness_script.clone().into_bytes()); - } else { return false; } - } + let chan_keys = TxCreationKeys::derive_new(&onchain_handler.secp_ctx, &outp.per_commitment_point, &outp.counterparty_delayed_payment_base_key, &outp.counterparty_htlc_base_key, &onchain_handler.signer.pubkeys().revocation_basepoint, &onchain_handler.signer.pubkeys().htlc_basepoint); + let witness_script = chan_utils::get_htlc_redeemscript_with_explicit_keys(&outp.htlc, onchain_handler.opt_anchors(), &chan_keys.broadcaster_htlc_key, &chan_keys.countersignatory_htlc_key, &chan_keys.revocation_key); + //TODO: should we panic on signer failure ? + if let Ok(sig) = onchain_handler.signer.sign_justice_revoked_htlc(&bumped_tx, i, outp.amount, &outp.per_commitment_key, &outp.htlc, &onchain_handler.secp_ctx) { + let mut ser_sig = sig.serialize_der().to_vec(); + ser_sig.push(EcdsaSighashType::All as u8); + bumped_tx.input[i].witness.push(ser_sig); + bumped_tx.input[i].witness.push(chan_keys.revocation_key.clone().serialize().to_vec()); + bumped_tx.input[i].witness.push(witness_script.clone().into_bytes()); + } else { return false; } }, PackageSolvingData::CounterpartyOfferedHTLCOutput(ref outp) => { - if let Ok(chan_keys) = TxCreationKeys::derive_new(&onchain_handler.secp_ctx, &outp.per_commitment_point, &outp.counterparty_delayed_payment_base_key, &outp.counterparty_htlc_base_key, &onchain_handler.signer.pubkeys().revocation_basepoint, &onchain_handler.signer.pubkeys().htlc_basepoint) { - let witness_script = chan_utils::get_htlc_redeemscript_with_explicit_keys(&outp.htlc, onchain_handler.opt_anchors(), &chan_keys.broadcaster_htlc_key, &chan_keys.countersignatory_htlc_key, &chan_keys.revocation_key); + let chan_keys = TxCreationKeys::derive_new(&onchain_handler.secp_ctx, &outp.per_commitment_point, &outp.counterparty_delayed_payment_base_key, &outp.counterparty_htlc_base_key, &onchain_handler.signer.pubkeys().revocation_basepoint, &onchain_handler.signer.pubkeys().htlc_basepoint); + let witness_script = chan_utils::get_htlc_redeemscript_with_explicit_keys(&outp.htlc, onchain_handler.opt_anchors(), &chan_keys.broadcaster_htlc_key, &chan_keys.countersignatory_htlc_key, &chan_keys.revocation_key); - if let Ok(sig) = onchain_handler.signer.sign_counterparty_htlc_transaction(&bumped_tx, i, &outp.htlc.amount_msat / 1000, &outp.per_commitment_point, &outp.htlc, &onchain_handler.secp_ctx) { - let mut ser_sig = sig.serialize_der().to_vec(); - ser_sig.push(EcdsaSighashType::All as u8); - bumped_tx.input[i].witness.push(ser_sig); - bumped_tx.input[i].witness.push(outp.preimage.0.to_vec()); - bumped_tx.input[i].witness.push(witness_script.clone().into_bytes()); - } + if let Ok(sig) = onchain_handler.signer.sign_counterparty_htlc_transaction(&bumped_tx, i, &outp.htlc.amount_msat / 1000, &outp.per_commitment_point, &outp.htlc, &onchain_handler.secp_ctx) { + let mut ser_sig = sig.serialize_der().to_vec(); + ser_sig.push(EcdsaSighashType::All as u8); + bumped_tx.input[i].witness.push(ser_sig); + bumped_tx.input[i].witness.push(outp.preimage.0.to_vec()); + bumped_tx.input[i].witness.push(witness_script.clone().into_bytes()); } }, PackageSolvingData::CounterpartyReceivedHTLCOutput(ref outp) => { - if let Ok(chan_keys) = TxCreationKeys::derive_new(&onchain_handler.secp_ctx, &outp.per_commitment_point, &outp.counterparty_delayed_payment_base_key, &outp.counterparty_htlc_base_key, &onchain_handler.signer.pubkeys().revocation_basepoint, &onchain_handler.signer.pubkeys().htlc_basepoint) { - let witness_script = chan_utils::get_htlc_redeemscript_with_explicit_keys(&outp.htlc, onchain_handler.opt_anchors(), &chan_keys.broadcaster_htlc_key, &chan_keys.countersignatory_htlc_key, &chan_keys.revocation_key); + let chan_keys = TxCreationKeys::derive_new(&onchain_handler.secp_ctx, &outp.per_commitment_point, &outp.counterparty_delayed_payment_base_key, &outp.counterparty_htlc_base_key, &onchain_handler.signer.pubkeys().revocation_basepoint, &onchain_handler.signer.pubkeys().htlc_basepoint); + let witness_script = chan_utils::get_htlc_redeemscript_with_explicit_keys(&outp.htlc, onchain_handler.opt_anchors(), &chan_keys.broadcaster_htlc_key, &chan_keys.countersignatory_htlc_key, &chan_keys.revocation_key); - bumped_tx.lock_time = PackedLockTime(outp.htlc.cltv_expiry); // Right now we don't aggregate time-locked transaction, if we do we should set lock_time before to avoid breaking hash computation - if let Ok(sig) = onchain_handler.signer.sign_counterparty_htlc_transaction(&bumped_tx, i, &outp.htlc.amount_msat / 1000, &outp.per_commitment_point, &outp.htlc, &onchain_handler.secp_ctx) { - let mut ser_sig = sig.serialize_der().to_vec(); - ser_sig.push(EcdsaSighashType::All as u8); - bumped_tx.input[i].witness.push(ser_sig); - // Due to BIP146 (MINIMALIF) this must be a zero-length element to relay. - bumped_tx.input[i].witness.push(vec![]); - bumped_tx.input[i].witness.push(witness_script.clone().into_bytes()); - } + bumped_tx.lock_time = PackedLockTime(outp.htlc.cltv_expiry); // Right now we don't aggregate time-locked transaction, if we do we should set lock_time before to avoid breaking hash computation + if let Ok(sig) = onchain_handler.signer.sign_counterparty_htlc_transaction(&bumped_tx, i, &outp.htlc.amount_msat / 1000, &outp.per_commitment_point, &outp.htlc, &onchain_handler.secp_ctx) { + let mut ser_sig = sig.serialize_der().to_vec(); + ser_sig.push(EcdsaSighashType::All as u8); + bumped_tx.input[i].witness.push(ser_sig); + // Due to BIP146 (MINIMALIF) this must be a zero-length element to relay. + bumped_tx.input[i].witness.push(vec![]); + bumped_tx.input[i].witness.push(witness_script.clone().into_bytes()); } }, _ => { panic!("API Error!"); } diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index 0b9381d2d..b5b9d2ee7 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -28,7 +28,6 @@ use crate::util::{byte_utils, transaction_utils}; use bitcoin::hash_types::WPubkeyHash; use bitcoin::secp256k1::{SecretKey, PublicKey, Scalar}; use bitcoin::secp256k1::{Secp256k1, ecdsa::Signature, Message}; -use bitcoin::secp256k1::Error as SecpError; use bitcoin::{PackedLockTime, secp256k1, Sequence, Witness}; use crate::io; @@ -483,19 +482,19 @@ impl_writeable_tlv_based!(ChannelPublicKeys, { impl TxCreationKeys { /// Create per-state keys from channel base points and the per-commitment point. /// Key set is asymmetric and can't be used as part of counter-signatory set of transactions. - pub fn derive_new(secp_ctx: &Secp256k1, per_commitment_point: &PublicKey, broadcaster_delayed_payment_base: &PublicKey, broadcaster_htlc_base: &PublicKey, countersignatory_revocation_base: &PublicKey, countersignatory_htlc_base: &PublicKey) -> Result { - Ok(TxCreationKeys { + pub fn derive_new(secp_ctx: &Secp256k1, per_commitment_point: &PublicKey, broadcaster_delayed_payment_base: &PublicKey, broadcaster_htlc_base: &PublicKey, countersignatory_revocation_base: &PublicKey, countersignatory_htlc_base: &PublicKey) -> TxCreationKeys { + TxCreationKeys { per_commitment_point: per_commitment_point.clone(), revocation_key: derive_public_revocation_key(&secp_ctx, &per_commitment_point, &countersignatory_revocation_base), broadcaster_htlc_key: derive_public_key(&secp_ctx, &per_commitment_point, &broadcaster_htlc_base), countersignatory_htlc_key: derive_public_key(&secp_ctx, &per_commitment_point, &countersignatory_htlc_base), broadcaster_delayed_payment_key: derive_public_key(&secp_ctx, &per_commitment_point, &broadcaster_delayed_payment_base), - }) + } } /// Generate per-state keys from channel static keys. /// Key set is asymmetric and can't be used as part of counter-signatory set of transactions. - pub fn from_channel_static_keys(per_commitment_point: &PublicKey, broadcaster_keys: &ChannelPublicKeys, countersignatory_keys: &ChannelPublicKeys, secp_ctx: &Secp256k1) -> Result { + pub fn from_channel_static_keys(per_commitment_point: &PublicKey, broadcaster_keys: &ChannelPublicKeys, countersignatory_keys: &ChannelPublicKeys, secp_ctx: &Secp256k1) -> TxCreationKeys { TxCreationKeys::derive_new( &secp_ctx, &per_commitment_point, @@ -1450,7 +1449,7 @@ impl CommitmentTransaction { pub fn verify(&self, channel_parameters: &DirectedChannelTransactionParameters, broadcaster_keys: &ChannelPublicKeys, countersignatory_keys: &ChannelPublicKeys, secp_ctx: &Secp256k1) -> Result { // This is the only field of the key cache that we trust let per_commitment_point = self.keys.per_commitment_point; - let keys = TxCreationKeys::from_channel_static_keys(&per_commitment_point, broadcaster_keys, countersignatory_keys, secp_ctx).unwrap(); + let keys = TxCreationKeys::from_channel_static_keys(&per_commitment_point, broadcaster_keys, countersignatory_keys, secp_ctx); if keys != self.keys { return Err(()); } @@ -1629,7 +1628,7 @@ mod tests { let htlc_basepoint = &signer.pubkeys().htlc_basepoint; let holder_pubkeys = signer.pubkeys(); let counterparty_pubkeys = counterparty_signer.pubkeys(); - let keys = TxCreationKeys::derive_new(&secp_ctx, &per_commitment_point, delayed_payment_base, htlc_basepoint, &counterparty_pubkeys.revocation_basepoint, &counterparty_pubkeys.htlc_basepoint).unwrap(); + let keys = TxCreationKeys::derive_new(&secp_ctx, &per_commitment_point, delayed_payment_base, htlc_basepoint, &counterparty_pubkeys.revocation_basepoint, &counterparty_pubkeys.htlc_basepoint); let mut channel_parameters = ChannelTransactionParameters { holder_pubkeys: holder_pubkeys.clone(), holder_selected_contest_delay: 0, diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index ef7c77f0b..dcbaa771f 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1749,27 +1749,27 @@ impl Channel { /// our counterparty!) /// The result is a transaction which we can revoke broadcastership of (ie a "local" transaction) /// TODO Some magic rust shit to compile-time check this? - fn build_holder_transaction_keys(&self, commitment_number: u64) -> Result { + fn build_holder_transaction_keys(&self, commitment_number: u64) -> TxCreationKeys { let per_commitment_point = self.holder_signer.get_per_commitment_point(commitment_number, &self.secp_ctx); let delayed_payment_base = &self.get_holder_pubkeys().delayed_payment_basepoint; let htlc_basepoint = &self.get_holder_pubkeys().htlc_basepoint; let counterparty_pubkeys = self.get_counterparty_pubkeys(); - Ok(secp_check!(TxCreationKeys::derive_new(&self.secp_ctx, &per_commitment_point, delayed_payment_base, htlc_basepoint, &counterparty_pubkeys.revocation_basepoint, &counterparty_pubkeys.htlc_basepoint), "Local tx keys generation got bogus keys".to_owned())) + TxCreationKeys::derive_new(&self.secp_ctx, &per_commitment_point, delayed_payment_base, htlc_basepoint, &counterparty_pubkeys.revocation_basepoint, &counterparty_pubkeys.htlc_basepoint) } #[inline] /// Creates a set of keys for build_commitment_transaction to generate a transaction which we /// will sign and send to our counterparty. /// If an Err is returned, it is a ChannelError::Close (for get_outbound_funding_created) - fn build_remote_transaction_keys(&self) -> Result { + fn build_remote_transaction_keys(&self) -> TxCreationKeys { //TODO: Ensure that the payment_key derived here ends up in the library users' wallet as we //may see payments to it! let revocation_basepoint = &self.get_holder_pubkeys().revocation_basepoint; let htlc_basepoint = &self.get_holder_pubkeys().htlc_basepoint; let counterparty_pubkeys = self.get_counterparty_pubkeys(); - Ok(secp_check!(TxCreationKeys::derive_new(&self.secp_ctx, &self.counterparty_cur_commitment_point.unwrap(), &counterparty_pubkeys.delayed_payment_basepoint, &counterparty_pubkeys.htlc_basepoint, revocation_basepoint, htlc_basepoint), "Remote tx keys generation got bogus keys".to_owned())) + TxCreationKeys::derive_new(&self.secp_ctx, &self.counterparty_cur_commitment_point.unwrap(), &counterparty_pubkeys.delayed_payment_basepoint, &counterparty_pubkeys.htlc_basepoint, revocation_basepoint, htlc_basepoint) } /// Gets the redeemscript for the funding transaction output (ie the funding transaction output @@ -2157,7 +2157,7 @@ impl Channel { fn funding_created_signature(&mut self, sig: &Signature, logger: &L) -> Result<(Txid, CommitmentTransaction, Signature), ChannelError> where L::Target: Logger { let funding_script = self.get_funding_redeemscript(); - let keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number)?; + let keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number); let initial_commitment_tx = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, false, logger).tx; { let trusted_tx = initial_commitment_tx.trust(); @@ -2171,7 +2171,7 @@ impl Channel { secp_check!(self.secp_ctx.verify_ecdsa(&sighash, &sig, self.counterparty_funding_pubkey()), "Invalid funding_created signature from peer".to_owned()); } - let counterparty_keys = self.build_remote_transaction_keys()?; + let counterparty_keys = self.build_remote_transaction_keys(); let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).tx; let counterparty_trusted_tx = counterparty_initial_commitment_tx.trust(); @@ -2285,7 +2285,7 @@ impl Channel { let funding_script = self.get_funding_redeemscript(); - let counterparty_keys = self.build_remote_transaction_keys()?; + let counterparty_keys = self.build_remote_transaction_keys(); let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).tx; let counterparty_trusted_tx = counterparty_initial_commitment_tx.trust(); let counterparty_initial_bitcoin_tx = counterparty_trusted_tx.built_transaction(); @@ -2293,7 +2293,7 @@ impl Channel { log_trace!(logger, "Initial counterparty tx for channel {} is: txid {} tx {}", log_bytes!(self.channel_id()), counterparty_initial_bitcoin_tx.txid, encode::serialize_hex(&counterparty_initial_bitcoin_tx.transaction)); - let holder_signer = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number)?; + let holder_signer = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number); let initial_commitment_tx = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &holder_signer, true, false, logger).tx; { let trusted_tx = initial_commitment_tx.trust(); @@ -2959,7 +2959,7 @@ impl Channel { let funding_script = self.get_funding_redeemscript(); - let keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number).map_err(|e| (None, e))?; + let keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number); let commitment_stats = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, false, logger); let commitment_txid = { @@ -3551,7 +3551,7 @@ impl Channel { // Before proposing a feerate update, check that we can actually afford the new fee. let inbound_stats = self.get_inbound_pending_htlc_stats(Some(feerate_per_kw)); let outbound_stats = self.get_outbound_pending_htlc_stats(Some(feerate_per_kw)); - let keys = if let Ok(keys) = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number) { keys } else { return None; }; + let keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number); let commitment_stats = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, true, logger); let buffer_fee_msat = Channel::::commit_tx_fee_sat(feerate_per_kw, commitment_stats.num_nondust_htlcs + outbound_stats.on_holder_tx_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.opt_anchors()) * 1000; let holder_balance_msat = commitment_stats.local_balance_msat - outbound_stats.holding_cell_msat; @@ -5221,7 +5221,7 @@ impl Channel { /// If an Err is returned, it is a ChannelError::Close (for get_outbound_funding_created) fn get_outbound_funding_created_signature(&mut self, logger: &L) -> Result where L::Target: Logger { - let counterparty_keys = self.build_remote_transaction_keys()?; + let counterparty_keys = self.build_remote_transaction_keys(); let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).tx; Ok(self.holder_signer.sign_counterparty_commitment(&counterparty_initial_commitment_tx, Vec::new(), &self.secp_ctx) .map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed".to_owned()))?.0) @@ -5522,7 +5522,7 @@ impl Channel { return Err(ChannelError::Ignore(format!("Cannot send value that would put us over the max HTLC value in flight our peer will accept ({})", self.counterparty_max_htlc_value_in_flight_msat))); } - let keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number)?; + let keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number); let commitment_stats = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, true, logger); if !self.is_outbound() { // Check that we won't violate the remote channel reserve by adding this HTLC. @@ -5714,7 +5714,7 @@ impl Channel { /// Only fails in case of bad keys. Used for channel_reestablish commitment_signed generation /// when we shouldn't change HTLC/channel state. fn send_commitment_no_state_update(&self, logger: &L) -> Result<(msgs::CommitmentSigned, (Txid, Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)>)), ChannelError> where L::Target: Logger { - let counterparty_keys = self.build_remote_transaction_keys()?; + let counterparty_keys = self.build_remote_transaction_keys(); let commitment_stats = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, true, logger); let counterparty_commitment_txid = commitment_stats.tx.trust().txid(); let (signature, htlc_signatures); @@ -7309,7 +7309,7 @@ mod tests { let per_commitment_secret = SecretKey::from_slice(&hex::decode("1f1e1d1c1b1a191817161514131211100f0e0d0c0b0a09080706050403020100").unwrap()[..]).unwrap(); let per_commitment_point = PublicKey::from_secret_key(&secp_ctx, &per_commitment_secret); let htlc_basepoint = &chan.holder_signer.pubkeys().htlc_basepoint; - let keys = TxCreationKeys::derive_new(&secp_ctx, &per_commitment_point, delayed_payment_base, htlc_basepoint, &counterparty_pubkeys.revocation_basepoint, &counterparty_pubkeys.htlc_basepoint).unwrap(); + let keys = TxCreationKeys::derive_new(&secp_ctx, &per_commitment_point, delayed_payment_base, htlc_basepoint, &counterparty_pubkeys.revocation_basepoint, &counterparty_pubkeys.htlc_basepoint); macro_rules! test_commitment { ( $counterparty_sig_hex: expr, $sig_hex: expr, $tx_hex: expr, $($remain:tt)* ) => { diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index d8dcb013e..d2f055dd5 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -705,7 +705,7 @@ fn test_update_fee_that_funder_cannot_afford() { // Assemble the set of keys we can use for signatures for our commitment_signed message. let commit_tx_keys = chan_utils::TxCreationKeys::derive_new(&secp_ctx, &remote_point, &remote_delayed_payment_basepoint, - &remote_htlc_basepoint, &local_revocation_basepoint, &local_htlc_basepoint).unwrap(); + &remote_htlc_basepoint, &local_revocation_basepoint, &local_htlc_basepoint); let res = { let local_chan_lock = nodes[0].node.channel_state.lock().unwrap(); @@ -1412,7 +1412,7 @@ fn test_fee_spike_violation_fails_htlc() { // Assemble the set of keys we can use for signatures for our commitment_signed message. let commit_tx_keys = chan_utils::TxCreationKeys::derive_new(&secp_ctx, &remote_point, &remote_delayed_payment_basepoint, - &remote_htlc_basepoint, &local_revocation_basepoint, &local_htlc_basepoint).unwrap(); + &remote_htlc_basepoint, &local_revocation_basepoint, &local_htlc_basepoint); // Build the remote commitment transaction so we can sign it, and then later use the // signature for the commitment_signed message.