From 399b3eb3f74b266e3273af04748a1cad46181494 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 7 Dec 2022 00:29:11 +0000 Subject: [PATCH 1/6] Use `PackageId` rather than `Txid` in `OnchainEvent::Claim` In 19daccf7fb5ea81c8d235c1628a91efe0aa07b96, a `PackageId` type was added to differentiate between an opaque Id for packages and the `Txid` type which was being used for that purpose. It, however, failed to also replace the single inner field in `OnchainEvent::Claim` which was also a package ID. We do so here. --- lightning/src/chain/onchaintx.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 5bf3a8fd482..c2ab8751660 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -79,7 +79,7 @@ enum OnchainEvent { /// Outpoint under claim process by our own tx, once this one get enough confirmations, we remove it from /// bump-txn candidate buffer. Claim { - claim_request: Txid, + package_id: PackageID, }, /// Claim tx aggregate multiple claimable outpoints. One of the outpoint may be claimed by a counterparty party tx. /// In this case, we need to drop the outpoint and regenerate a new claim tx. By safety, we keep tracking @@ -123,7 +123,7 @@ impl MaybeReadable for OnchainEventEntry { impl_writeable_tlv_based_enum_upgradable!(OnchainEvent, (0, Claim) => { - (0, claim_request, required), + (0, package_id, required), }, (1, ContentiousOutpoint) => { (0, package, required), @@ -480,8 +480,8 @@ impl OnchainTxHandler { // We check for outpoint spends within claims individually rather than as a set // since requests can have outpoints split off. if !self.onchain_events_awaiting_threshold_conf.iter() - .any(|event_entry| if let OnchainEvent::Claim { claim_request } = event_entry.event { - first_claim_txid_height.0 == claim_request.into_inner() + .any(|event_entry| if let OnchainEvent::Claim { package_id } = event_entry.event { + first_claim_txid_height.0 == package_id } else { // The onchain event is not a claim, keep seeking until we find one. false @@ -766,7 +766,7 @@ impl OnchainTxHandler { txid: tx.txid(), height: conf_height, block_hash: Some(conf_hash), - event: OnchainEvent::Claim { claim_request: Txid::from_inner(first_claim_txid_height.0) } + event: OnchainEvent::Claim { package_id: first_claim_txid_height.0 } }; if !self.onchain_events_awaiting_threshold_conf.contains(&entry) { self.onchain_events_awaiting_threshold_conf.push(entry); @@ -821,13 +821,13 @@ impl OnchainTxHandler { for entry in onchain_events_awaiting_threshold_conf { if entry.has_reached_confirmation_threshold(cur_height) { match entry.event { - OnchainEvent::Claim { claim_request } => { - let package_id = claim_request.into_inner(); + OnchainEvent::Claim { package_id } => { // We may remove a whole set of claim outpoints here, as these one may have // been aggregated in a single tx and claimed so atomically if let Some(request) = self.pending_claim_requests.remove(&package_id) { for outpoint in request.outpoints() { - log_debug!(logger, "Removing claim tracking for {} due to maturation of claim tx {}.", outpoint, claim_request); + log_debug!(logger, "Removing claim tracking for {} due to maturation of claim package {}.", + outpoint, log_bytes!(package_id)); self.claimable_outpoints.remove(&outpoint); #[cfg(anchors)] self.pending_claim_events.remove(&package_id); From 60f3c0a206bfdc851400bce7753db688b01b094e Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 7 Dec 2022 00:30:43 +0000 Subject: [PATCH 2/6] DRY the comparison blocks in `update_claims_view_from_matched_txn` In `update_claims_view_from_matched_txn` we have two different tx-equivalence checks which do the same thing - both check that the tx which appeared on chain spent all of the outpoints which we intended to spend in a given package. While one is more effecient than the other (but only usable in a subset of cases), the difference between O(N) and O(N^2) when N is 1-5 is trivial. Still, it is possible we hit this code with just shy of 900 HTLC outputs in a channel, and a transaction with a ton of inputs. While having to spin through a few million entries if our counterparty wastes a full block isn't really a big deal, we go ahead and use a sorted vec and binary searches because its trivial. --- lightning/src/chain/onchaintx.rs | 28 +++++----------------------- 1 file changed, 5 insertions(+), 23 deletions(-) diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index c2ab8751660..b75349469f8 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -733,31 +733,13 @@ impl OnchainTxHandler { // outpoints to know if transaction is the original claim or a bumped one issued // by us. let mut are_sets_equal = true; - if !request.requires_external_funding() || !request.is_malleable() { - // If the claim does not require external funds to be allocated through - // additional inputs we can simply check the inputs in order as they - // cannot change under us. - if request.outpoints().len() != tx.input.len() { + let mut tx_inputs = tx.input.iter().map(|input| &input.previous_output).collect::>(); + tx_inputs.sort_unstable(); + for request_input in request.outpoints() { + if tx_inputs.binary_search(&request_input).is_err() { are_sets_equal = false; - } else { - for (claim_inp, tx_inp) in request.outpoints().iter().zip(tx.input.iter()) { - if **claim_inp != tx_inp.previous_output { - are_sets_equal = false; - } - } - } - } else { - // Otherwise, we'll do a linear search for each input (we don't expect - // large input sets to exist) to ensure the request's input set is fully - // spent to be resilient against the external claim reordering inputs. - let mut spends_all_inputs = true; - for request_input in request.outpoints() { - if tx.input.iter().find(|input| input.previous_output == *request_input).is_none() { - spends_all_inputs = false; - break; - } + break; } - are_sets_equal = spends_all_inputs; } macro_rules! clean_claim_request_after_safety_delay { From 02235782f04308c47a2e13becaeff972cab52185 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 7 Dec 2022 00:40:17 +0000 Subject: [PATCH 3/6] Drop excess mut on `OnchainTxHandler::generate_external_htlc_claim` --- lightning/src/chain/onchaintx.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index b75349469f8..9bfe6fa9af2 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -1047,7 +1047,7 @@ impl OnchainTxHandler { #[cfg(anchors)] pub(crate) fn generate_external_htlc_claim( - &mut self, outp: &::bitcoin::OutPoint, preimage: &Option + &self, outp: &::bitcoin::OutPoint, preimage: &Option ) -> Option { let find_htlc = |holder_commitment: &HolderCommitmentTransaction| -> Option { let trusted_tx = holder_commitment.trust(); From 381e8b96789ac4dbff2315aeb88d8b77d7bd3cfa Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 7 Dec 2022 00:40:38 +0000 Subject: [PATCH 4/6] Use `Witness::push_bitcoin_signature` where relevant --- lightning/src/ln/chan_utils.rs | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index 729425d70d5..408f1cd7e47 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -739,17 +739,12 @@ pub fn build_htlc_input_witness( } else { EcdsaSighashType::All }; - let mut remote_sig = remote_sig.serialize_der().to_vec(); - remote_sig.push(remote_sighash_type as u8); - - let mut local_sig = local_sig.serialize_der().to_vec(); - local_sig.push(EcdsaSighashType::All as u8); let mut witness = Witness::new(); // First push the multisig dummy, note that due to BIP147 (NULLDUMMY) it must be a zero-length element. witness.push(vec![]); - witness.push(remote_sig); - witness.push(local_sig); + witness.push_bitcoin_signature(&remote_sig.serialize_der(), remote_sighash_type); + witness.push_bitcoin_signature(&local_sig.serialize_der(), EcdsaSighashType::All); if let Some(preimage) = preimage { witness.push(preimage.0.to_vec()); } else { @@ -801,9 +796,10 @@ pub(crate) fn get_anchor_output<'a>(commitment_tx: &'a Transaction, funding_pubk /// Returns the witness required to satisfy and spend an anchor input. pub fn build_anchor_input_witness(funding_key: &PublicKey, funding_sig: &Signature) -> Witness { let anchor_redeem_script = chan_utils::get_anchor_redeemscript(funding_key); - let mut funding_sig = funding_sig.serialize_der().to_vec(); - funding_sig.push(EcdsaSighashType::All as u8); - Witness::from_vec(vec![funding_sig, anchor_redeem_script.to_bytes()]) + let mut ret = Witness::new(); + ret.push_bitcoin_signature(&funding_sig.serialize_der(), EcdsaSighashType::All); + ret.push(anchor_redeem_script.as_bytes()); + ret } /// Per-channel data used to build transactions in conjunction with the per-commitment data (CommitmentTransaction). @@ -1037,17 +1033,13 @@ impl HolderCommitmentTransaction { // First push the multisig dummy, note that due to BIP147 (NULLDUMMY) it must be a zero-length element. let mut tx = self.inner.built.transaction.clone(); tx.input[0].witness.push(Vec::new()); - let mut ser_holder_sig = holder_sig.serialize_der().to_vec(); - ser_holder_sig.push(EcdsaSighashType::All as u8); - let mut ser_cp_sig = self.counterparty_sig.serialize_der().to_vec(); - ser_cp_sig.push(EcdsaSighashType::All as u8); if self.holder_sig_first { - tx.input[0].witness.push(ser_holder_sig); - tx.input[0].witness.push(ser_cp_sig); + tx.input[0].witness.push_bitcoin_signature(&holder_sig.serialize_der(), EcdsaSighashType::All); + tx.input[0].witness.push_bitcoin_signature(&self.counterparty_sig.serialize_der(), EcdsaSighashType::All); } else { - tx.input[0].witness.push(ser_cp_sig); - tx.input[0].witness.push(ser_holder_sig); + tx.input[0].witness.push_bitcoin_signature(&self.counterparty_sig.serialize_der(), EcdsaSighashType::All); + tx.input[0].witness.push_bitcoin_signature(&holder_sig.serialize_der(), EcdsaSighashType::All); } tx.input[0].witness.push(funding_redeemscript.as_bytes().to_vec()); From a1f7cbb572fa50dcb933bb162facb5ce2cd82576 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 7 Dec 2022 00:41:07 +0000 Subject: [PATCH 5/6] Ensure `Event::BumpTransaction` is forwards-compatible `Event`s with an odd type are ignored by older versions of LDK, however they rely on the `write_tlv_fields` call to know how many bytes to read and discard. We were missing that call in our writing of `Event::BumpTransaction`, which we add here. --- lightning/src/util/events.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs index 2c7e5413daf..375174f6f6a 100644 --- a/lightning/src/util/events.rs +++ b/lightning/src/util/events.rs @@ -1125,6 +1125,7 @@ impl Writeable for Event { BumpTransactionEvent::ChannelClose { .. } => {} BumpTransactionEvent::HTLCResolution { .. } => {} } + write_tlv_fields!(writer, {}); // Write a length field for forwards compat } &Event::ChannelReady { ref channel_id, ref user_channel_id, ref counterparty_node_id, ref channel_type } => { 29u8.write(writer)?; From 67f9f01ac921bb9228300a9887d52984b2d03c66 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 7 Dec 2022 23:17:31 +0000 Subject: [PATCH 6/6] Slightly clarify comment on safety of only processing HTLCs once --- lightning/src/chain/channelmonitor.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 5c1e102aefc..a41d853311c 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -3032,10 +3032,10 @@ impl ChannelMonitorImpl { if let Some(new_outputs) = new_outputs_option { watch_outputs.push(new_outputs); } - // Since there may be multiple HTLCs (all from the same commitment) being - // claimed by the counterparty within the same transaction, and - // `check_spend_counterparty_htlc` already checks for all of them, we can - // safely break from our loop. + // Since there may be multiple HTLCs for this channel (all spending the + // same commitment tx) being claimed by the counterparty within the same + // transaction, and `check_spend_counterparty_htlc` already checks all the + // ones relevant to this channel, we can safely break from our loop. break; } }