From 599143138ec7d1947021d886cf5b42d8dc68f6af Mon Sep 17 00:00:00 2001 From: Rachel Malonson Date: Thu, 28 Sep 2023 15:05:07 -0700 Subject: [PATCH 1/3] Remove panics for sign_holder_commitment_and_htlcs --- lightning/src/chain/channelmonitor.rs | 104 ++++++++++++---------- lightning/src/chain/onchaintx.rs | 9 +- lightning/src/chain/package.rs | 15 +++- lightning/src/ln/functional_test_utils.rs | 2 +- lightning/src/sign/errors.rs | 20 +++++ lightning/src/sign/mod.rs | 16 ++-- lightning/src/util/test_channel_signer.rs | 5 +- 7 files changed, 112 insertions(+), 59 deletions(-) create mode 100644 lightning/src/sign/errors.rs diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 3f9c83bb543..b4551f96f5d 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -43,6 +43,7 @@ use crate::chain; use crate::chain::{BestBlock, WatchedOutput}; use crate::chain::chaininterface::{BroadcasterInterface, FeeEstimator, LowerBoundedFeeEstimator}; use crate::chain::transaction::{OutPoint, TransactionData}; +use crate::sign::errors::SigningError; use crate::sign::{SpendableOutputDescriptor, StaticPaymentOutputDescriptor, DelayedPaymentOutputDescriptor, WriteableEcdsaChannelSigner, SignerProvider, EntropySource}; use crate::chain::onchaintx::{ClaimEvent, OnchainTxHandler}; use crate::chain::package::{CounterpartyOfferedHTLCOutput, CounterpartyReceivedHTLCOutput, HolderFundingOutput, HolderHTLCOutput, PackageSolvingData, PackageTemplate, RevokedOutput, RevokedHTLCOutput}; @@ -1482,21 +1483,16 @@ impl ChannelMonitor { self.inner.lock().unwrap().counterparty_node_id } - /// Used by [`ChannelManager`] deserialization to broadcast the latest holder state if its copy - /// of the channel state was out-of-date. - /// - /// You may also use this to broadcast the latest local commitment transaction, either because + /// You may use this to broadcast the latest local commitment transaction, either because /// a monitor update failed or because we've fallen behind (i.e. we've received proof that our /// counterparty side knows a revocation secret we gave them that they shouldn't know). /// - /// Broadcasting these transactions in the second case is UNSAFE, as they allow counterparty + /// Broadcasting these transactions in this manner is UNSAFE, as they allow counterparty /// side to punish you. Nevertheless you may want to broadcast them if counterparty doesn't /// close channel with their commitment transaction after a substantial amount of time. Best /// may be to contact the other node operator out-of-band to coordinate other options available /// to you. - /// - /// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager - pub fn get_latest_holder_commitment_txn(&self, logger: &L) -> Vec + pub fn get_latest_holder_commitment_txn(&self, logger: &L) -> Result, SigningError> where L::Target: Logger { self.inner.lock().unwrap().get_latest_holder_commitment_txn(logger) } @@ -1505,7 +1501,7 @@ impl ChannelMonitor { /// to bypass HolderCommitmentTransaction state update lockdown after signature and generate /// revoked commitment transaction. #[cfg(any(test, feature = "unsafe_revoked_tx_signing"))] - pub fn unsafe_get_latest_holder_commitment_txn(&self, logger: &L) -> Vec + pub fn unsafe_get_latest_holder_commitment_txn(&self, logger: &L) -> Result, SigningError> where L::Target: Logger { self.inner.lock().unwrap().unsafe_get_latest_holder_commitment_txn(logger) } @@ -2515,18 +2511,53 @@ impl ChannelMonitorImpl { } } - pub(crate) fn broadcast_latest_holder_commitment_txn(&mut self, broadcaster: &B, logger: &L) + fn generate_claimable_outpoints_and_watch_outputs(&mut self) -> (Vec, Vec) { + let funding_outp = HolderFundingOutput::build( + self.funding_redeemscript.clone(), + self.channel_value_satoshis, + self.onchain_tx_handler.channel_type_features().clone() + ); + let commitment_package = PackageTemplate::build_package( + self.funding_info.0.txid.clone(), self.funding_info.0.index as u32, + PackageSolvingData::HolderFundingOutput(funding_outp), + self.best_block.height(), self.best_block.height() + ); + let mut claimable_outpoints = vec![commitment_package]; + self.pending_monitor_events.push(MonitorEvent::HolderForceClosed(self.funding_info.0)); + // Although we aren't signing the transaction directly here, the transaction will be signed + // in the claim that is queued to OnchainTxHandler. We set holder_tx_signed here to reject + // new channel updates. + self.holder_tx_signed = true; + let mut watch_outputs = Vec::new(); + // We can't broadcast our HTLC transactions while the commitment transaction is + // unconfirmed. We'll delay doing so until we detect the confirmed commitment in + // `transactions_confirmed`. + if !self.onchain_tx_handler.channel_type_features().supports_anchors_zero_fee_htlc_tx() { + // Because we're broadcasting a commitment transaction, we should construct the package + // assuming it gets confirmed in the next block. Sadly, we have code which considers + // "not yet confirmed" things as discardable, so we cannot do that here. + let (mut new_outpoints, _) = self.get_broadcasted_holder_claims( + &self.current_holder_commitment_tx, self.best_block.height() + ); + let unsigned_commitment_tx = self.onchain_tx_handler.get_unsigned_holder_commitment_tx(); + let new_outputs = self.get_broadcasted_holder_watch_outputs( + &self.current_holder_commitment_tx, &unsigned_commitment_tx + ); + if !new_outputs.is_empty() { + watch_outputs.push((self.current_holder_commitment_tx.txid.clone(), new_outputs)); + } + claimable_outpoints.append(&mut new_outpoints); + } + (claimable_outpoints, watch_outputs) + } + + pub(crate) fn queue_latest_holder_commitment_txn_for_broadcast(&mut self, broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator, logger: &L) where B::Target: BroadcasterInterface, - L::Target: Logger, + F::Target: FeeEstimator, + L::Target: Logger, { - let commit_txs = self.get_latest_holder_commitment_txn(logger); - let mut txs = vec![]; - for tx in commit_txs.iter() { - log_info!(logger, "Broadcasting local {}", log_tx!(tx)); - txs.push(tx); - } - broadcaster.broadcast_transactions(&txs); - self.pending_monitor_events.push(MonitorEvent::HolderForceClosed(self.funding_info.0)); + let (claimable_outpoints, _) = self.generate_claimable_outpoints_and_watch_outputs(); + self.onchain_tx_handler.update_claims_view_from_requests(claimable_outpoints, self.best_block.height(), self.best_block.height(), broadcaster, fee_estimator, logger); } pub fn update_monitor(&mut self, updates: &ChannelMonitorUpdate, broadcaster: &B, fee_estimator: F, logger: &L) -> Result<(), ()> @@ -2614,26 +2645,7 @@ impl ChannelMonitorImpl { log_trace!(logger, "Avoiding commitment broadcast, already detected confirmed spend onchain"); continue; } - self.broadcast_latest_holder_commitment_txn(broadcaster, logger); - // If the channel supports anchor outputs, we'll need to emit an external - // event to be consumed such that a child transaction is broadcast with a - // high enough feerate for the parent commitment transaction to confirm. - if self.onchain_tx_handler.channel_type_features().supports_anchors_zero_fee_htlc_tx() { - let funding_output = HolderFundingOutput::build( - self.funding_redeemscript.clone(), self.channel_value_satoshis, - self.onchain_tx_handler.channel_type_features().clone(), - ); - let best_block_height = self.best_block.height(); - let commitment_package = PackageTemplate::build_package( - self.funding_info.0.txid.clone(), self.funding_info.0.index as u32, - PackageSolvingData::HolderFundingOutput(funding_output), - best_block_height, best_block_height - ); - self.onchain_tx_handler.update_claims_view_from_requests( - vec![commitment_package], best_block_height, best_block_height, - broadcaster, &bounded_fee_estimator, logger, - ); - } + self.queue_latest_holder_commitment_txn_for_broadcast(broadcaster, &bounded_fee_estimator, logger); } else if !self.holder_tx_signed { log_error!(logger, "WARNING: You have a potentially-unsafe holder commitment transaction available to broadcast"); log_error!(logger, " in channel monitor for channel {}!", &self.funding_info.0.to_channel_id()); @@ -3211,16 +3223,16 @@ impl ChannelMonitorImpl { } } - pub fn get_latest_holder_commitment_txn(&mut self, logger: &L) -> Vec where L::Target: Logger { + pub fn get_latest_holder_commitment_txn(&mut self, logger: &L) -> Result, SigningError> where L::Target: Logger { log_debug!(logger, "Getting signed latest holder commitment transaction!"); self.holder_tx_signed = true; - let commitment_tx = self.onchain_tx_handler.get_fully_signed_holder_tx(&self.funding_redeemscript); + let commitment_tx = self.onchain_tx_handler.get_fully_signed_holder_tx(&self.funding_redeemscript)?; let txid = commitment_tx.txid(); let mut holder_transactions = vec![commitment_tx]; // When anchor outputs are present, the HTLC transactions are only valid once the commitment // transaction confirms. if self.onchain_tx_handler.channel_type_features().supports_anchors_zero_fee_htlc_tx() { - return holder_transactions; + return Ok(holder_transactions); } for htlc in self.current_holder_commitment_tx.htlc_outputs.iter() { if let Some(vout) = htlc.0.transaction_output_index { @@ -3245,12 +3257,12 @@ impl ChannelMonitorImpl { } // We throw away the generated waiting_first_conf data as we aren't (yet) confirmed and we don't actually know what the caller wants to do. // The data will be re-generated and tracked in check_spend_holder_transaction if we get a confirmation. - holder_transactions + Ok(holder_transactions) } #[cfg(any(test,feature = "unsafe_revoked_tx_signing"))] /// Note that this includes possibly-locktimed-in-the-future transactions! - fn unsafe_get_latest_holder_commitment_txn(&mut self, logger: &L) -> Vec where L::Target: Logger { + fn unsafe_get_latest_holder_commitment_txn(&mut self, logger: &L) -> Result, SigningError> where L::Target: Logger { log_debug!(logger, "Getting signed copy of latest holder commitment transaction!"); let commitment_tx = self.onchain_tx_handler.get_fully_signed_copy_holder_tx(&self.funding_redeemscript); let txid = commitment_tx.txid(); @@ -3258,7 +3270,7 @@ impl ChannelMonitorImpl { // When anchor outputs are present, the HTLC transactions are only final once the commitment // transaction confirms due to the CSV 1 encumberance. if self.onchain_tx_handler.channel_type_features().supports_anchors_zero_fee_htlc_tx() { - return holder_transactions; + return Ok(holder_transactions); } for htlc in self.current_holder_commitment_tx.htlc_outputs.iter() { if let Some(vout) = htlc.0.transaction_output_index { @@ -3274,7 +3286,7 @@ impl ChannelMonitorImpl { } } } - holder_transactions + Ok(holder_transactions) } pub fn block_connected(&mut self, header: &BlockHeader, txdata: &TransactionData, height: u32, broadcaster: B, fee_estimator: F, logger: L) -> Vec diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 5daa2463de9..384e8b556c9 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -30,6 +30,7 @@ use crate::ln::chan_utils::{self, ChannelTransactionParameters, HTLCOutputInComm use crate::chain::ClaimId; use crate::chain::chaininterface::{ConfirmationTarget, FeeEstimator, BroadcasterInterface, LowerBoundedFeeEstimator}; use crate::chain::channelmonitor::{ANTI_REORG_DELAY, CLTV_SHARED_CLAIM_BUFFER}; +use crate::sign::errors::SigningError; use crate::sign::WriteableEcdsaChannelSigner; use crate::chain::package::{PackageSolvingData, PackageTemplate}; use crate::util::logger::Logger; @@ -1131,10 +1132,12 @@ impl OnchainTxHandler // have empty holder commitment transaction if a ChannelMonitor is asked to force-close just after OutboundV1Channel::get_funding_created, // before providing a initial commitment transaction. For outbound channel, init ChannelMonitor at Channel::funding_signed, there is nothing // to monitor before. - pub(crate) fn get_fully_signed_holder_tx(&mut self, funding_redeemscript: &Script) -> Transaction { - let (sig, htlc_sigs) = self.signer.sign_holder_commitment_and_htlcs(&self.holder_commitment, &self.secp_ctx).expect("signing holder commitment"); + pub(crate) fn get_fully_signed_holder_tx(&mut self, funding_redeemscript: &Script) -> Result { + let (sig, htlc_sigs) = self.signer.sign_holder_commitment_and_htlcs( + &self.holder_commitment, &self.secp_ctx + )?; self.holder_htlc_sigs = Some(Self::extract_holder_sigs(&self.holder_commitment, htlc_sigs)); - self.holder_commitment.add_holder_sig(funding_redeemscript, sig) + Ok(self.holder_commitment.add_holder_sig(funding_redeemscript, sig)) } #[cfg(any(test, feature="unsafe_revoked_tx_signing"))] diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index 382ffac3e8e..9b40fe99c19 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -25,6 +25,7 @@ use crate::ln::chan_utils::{TxCreationKeys, HTLCOutputInCommitment}; use crate::ln::chan_utils; use crate::ln::msgs::DecodeError; use crate::chain::chaininterface::{FeeEstimator, ConfirmationTarget, MIN_RELAY_FEE_SAT_PER_1000_WEIGHT}; +use crate::sign::errors::SigningError; use crate::sign::WriteableEcdsaChannelSigner; use crate::chain::onchaintx::{ExternalHTLCClaim, OnchainTxHandler}; use crate::util::logger::Logger; @@ -613,7 +614,19 @@ impl PackageSolvingData { return onchain_handler.get_fully_signed_htlc_tx(outpoint, &outp.preimage); } PackageSolvingData::HolderFundingOutput(ref outp) => { - return Some(onchain_handler.get_fully_signed_holder_tx(&outp.funding_redeemscript)); + match onchain_handler.get_fully_signed_holder_tx(&outp.funding_redeemscript) { + Ok(tx) => { + return Some(tx); + }, + Err(e) => match e{ + SigningError::NotAvailable => { + return None; + }, + SigningError::PermanentFailure => { + panic!("Failed to sign holder transaction!"); + }, + }, + } } _ => { panic!("API Error!"); } } diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 82fa06f5c80..c71bacaa1f0 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -885,7 +885,7 @@ macro_rules! get_monitor { macro_rules! get_local_commitment_txn { ($node: expr, $channel_id: expr) => { { - $crate::get_monitor!($node, $channel_id).unsafe_get_latest_holder_commitment_txn(&$node.logger) + $crate::get_monitor!($node, $channel_id).unsafe_get_latest_holder_commitment_txn(&$node.logger).unwrap() } } } diff --git a/lightning/src/sign/errors.rs b/lightning/src/sign/errors.rs new file mode 100644 index 00000000000..2a1b3f331fa --- /dev/null +++ b/lightning/src/sign/errors.rs @@ -0,0 +1,20 @@ +// This file is Copyright its original authors, visible in version control +// history. +// +// This file is licensed under the Apache License, Version 2.0 or the MIT license +// , at your option. +// You may not use this file except in accordance with one or both of these +// licenses. + +//! Signing error types live here. + +#[derive(Clone, Debug)] +pub enum SigningError { + /// The signature is not immediately available from the signer but will be + /// provided later when the signer is online. + NotAvailable, + /// The signer failed permanently and we should attempt to close the + /// channel. + PermanentFailure, +} \ No newline at end of file diff --git a/lightning/src/sign/mod.rs b/lightning/src/sign/mod.rs index f25c9476448..5b2961477a6 100644 --- a/lightning/src/sign/mod.rs +++ b/lightning/src/sign/mod.rs @@ -56,6 +56,8 @@ use crate::util::atomic_counter::AtomicCounter; use crate::util::chacha20::ChaCha20; use crate::util::invoice::construct_invoice_preimage; +pub mod errors; + pub(crate) mod type_resolver; /// Used as initial key material, to be expanded into multiple secret keys (but not to be used @@ -446,14 +448,14 @@ pub trait EcdsaChannelSigner: ChannelSigner { /// [`ChannelMonitor`]: crate::chain::channelmonitor::ChannelMonitor // TODO: Document the things someone using this interface should enforce before signing. fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, - secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()>; + secp_ctx: &Secp256k1) -> Result<(Signature, Vec), errors::SigningError>; /// Same as [`sign_holder_commitment_and_htlcs`], but exists only for tests to get access to /// holder commitment transactions which will be broadcasted later, after the channel has moved /// on to a newer state. Thus, needs its own method as [`sign_holder_commitment_and_htlcs`] may /// enforce that we only ever get called once. #[cfg(any(test,feature = "unsafe_revoked_tx_signing"))] fn unsafe_sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, - secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()>; + secp_ctx: &Secp256k1) -> Result<(Signature, Vec), errors::SigningError>; /// Create a signature for the given input in a transaction spending an HTLC transaction output /// or a commitment transaction `to_local` output when our counterparty broadcasts an old state. /// @@ -1012,24 +1014,26 @@ impl EcdsaChannelSigner for InMemorySigner { Ok(()) } - fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()> { + fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), errors::SigningError> { let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key); let funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &self.counterparty_pubkeys().funding_pubkey); let trusted_tx = commitment_tx.trust(); let sig = trusted_tx.built_transaction().sign_holder_commitment(&self.funding_key, &funding_redeemscript, self.channel_value_satoshis, &self, secp_ctx); let channel_parameters = self.get_channel_parameters(); - let htlc_sigs = trusted_tx.get_htlc_sigs(&self.htlc_base_key, &channel_parameters.as_holder_broadcastable(), &self, secp_ctx)?; + let htlc_sigs = trusted_tx.get_htlc_sigs(&self.htlc_base_key, &channel_parameters.as_holder_broadcastable(), &self, secp_ctx) + .map_err(|_| errors::SigningError::PermanentFailure)?; Ok((sig, htlc_sigs)) } #[cfg(any(test,feature = "unsafe_revoked_tx_signing"))] - fn unsafe_sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()> { + fn unsafe_sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), errors::SigningError> { let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key); let funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &self.counterparty_pubkeys().funding_pubkey); let trusted_tx = commitment_tx.trust(); let sig = trusted_tx.built_transaction().sign_holder_commitment(&self.funding_key, &funding_redeemscript, self.channel_value_satoshis, &self, secp_ctx); let channel_parameters = self.get_channel_parameters(); - let htlc_sigs = trusted_tx.get_htlc_sigs(&self.htlc_base_key, &channel_parameters.as_holder_broadcastable(), &self, secp_ctx)?; + let htlc_sigs = trusted_tx.get_htlc_sigs(&self.htlc_base_key, &channel_parameters.as_holder_broadcastable(), &self, secp_ctx) + .map_err(|_| errors::SigningError::PermanentFailure)?; Ok((sig, htlc_sigs)) } diff --git a/lightning/src/util/test_channel_signer.rs b/lightning/src/util/test_channel_signer.rs index 2fb1c494f93..e214610860a 100644 --- a/lightning/src/util/test_channel_signer.rs +++ b/lightning/src/util/test_channel_signer.rs @@ -27,6 +27,7 @@ use crate::events::bump_transaction::HTLCDescriptor; use crate::util::ser::{Writeable, Writer}; use crate::io::Error; use crate::ln::features::ChannelTypeFeatures; +use crate::sign::errors::SigningError; /// Initial value for revoked commitment downward counter pub const INITIAL_REVOKED_COMMITMENT_NUMBER: u64 = 1 << 48; @@ -155,7 +156,7 @@ impl EcdsaChannelSigner for TestChannelSigner { Ok(()) } - fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()> { + fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), SigningError> { let trusted_tx = self.verify_holder_commitment_tx(commitment_tx, secp_ctx); let commitment_txid = trusted_tx.txid(); let holder_csv = self.inner.counterparty_selected_contest_delay(); @@ -193,7 +194,7 @@ impl EcdsaChannelSigner for TestChannelSigner { } #[cfg(any(test,feature = "unsafe_revoked_tx_signing"))] - fn unsafe_sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()> { + fn unsafe_sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), SigningError> { Ok(self.inner.unsafe_sign_holder_commitment_and_htlcs(commitment_tx, secp_ctx).unwrap()) } From 6e1ad1b45002dd96f8ebaff59d72d62949d5c2b3 Mon Sep 17 00:00:00 2001 From: Rachel Malonson Date: Wed, 4 Oct 2023 16:04:58 -0700 Subject: [PATCH 2/3] Fix some tests --- lightning/src/ln/functional_test_utils.rs | 5 -- lightning/src/ln/functional_tests.rs | 70 +++++++++++++++++------ lightning/src/ln/monitor_tests.rs | 12 +++- lightning/src/ln/payment_tests.rs | 53 +++++++++++++---- 4 files changed, 104 insertions(+), 36 deletions(-) diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index c71bacaa1f0..56fe1c8062e 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -2788,9 +2788,6 @@ pub enum HTLCType { NONE, TIMEOUT, SUCCESS } /// /// Next tests that there is (or is not) a transaction that spends the commitment transaction /// that appears to be the type of HTLC transaction specified in has_htlc_tx. -/// -/// All broadcast transactions must be accounted for in one of the above three types of we'll -/// also fail. pub fn test_txn_broadcast<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, chan: &(msgs::ChannelUpdate, msgs::ChannelUpdate, ChannelId, Transaction), commitment_tx: Option, has_htlc_tx: HTLCType) -> Vec { let mut node_txn = node.tx_broadcaster.txn_broadcasted.lock().unwrap(); let mut txn_seen = HashSet::new(); @@ -2831,8 +2828,6 @@ pub fn test_txn_broadcast<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, chan: &(msgs::Cha assert_eq!(res[1], res[2]); } } - - assert!(node_txn.is_empty()); res } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 1066362a4c4..ff6cea85a66 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -3561,10 +3561,19 @@ fn test_htlc_ignore_latest_remote_commitment() { check_closed_event!(nodes[0], 1, ClosureReason::HolderForceClosed, [nodes[1].node.get_our_node_id()], 100000); let node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); - assert_eq!(node_txn.len(), 3); - assert_eq!(node_txn[0].txid(), node_txn[1].txid()); + assert!(node_txn.len() >= 3); - let block = create_dummy_block(nodes[1].best_block_hash(), 42, vec![node_txn[0].clone(), node_txn[1].clone()]); + let mut node_txn_1 = &node_txn[0]; + let mut node_txn_2 = &node_txn[1]; + for n in 1..node_txn.len() { + if node_txn[n].txid() == node_txn_1.txid() { + node_txn_2 = &node_txn[n]; + break; + } + } + assert_eq!(node_txn_1.txid(), node_txn_2.txid()); + + let block = create_dummy_block(nodes[1].best_block_hash(), 42, vec![node_txn[0].clone(), node_txn_2.clone()]); connect_block(&nodes[1], &block); check_closed_broadcast!(nodes[1], true); check_added_monitors!(nodes[1], 1); @@ -3644,13 +3653,21 @@ fn test_force_close_fail_back() { } mine_transaction(&nodes[2], &tx); let node_txn = nodes[2].tx_broadcaster.txn_broadcasted.lock().unwrap(); - assert_eq!(node_txn.len(), 1); - assert_eq!(node_txn[0].input.len(), 1); - assert_eq!(node_txn[0].input[0].previous_output.txid, tx.txid()); - assert_eq!(node_txn[0].lock_time.0, 0); // Must be an HTLC-Success - assert_eq!(node_txn[0].input[0].witness.len(), 5); // Must be an HTLC-Success + assert!(node_txn.len() >= 1); + // Multiple tx may be broadcasted here, so we loop to find the one matching the expected txid. + let mut expected_node_tx = &node_txn[0]; + for node_tx in node_txn.iter() { + if node_tx.input[0].previous_output.txid == tx.txid() { + expected_node_tx = node_tx; + break; + } + } + assert_eq!(expected_node_tx.input.len(), 1); + assert_eq!(expected_node_tx.input[0].previous_output.txid, tx.txid()); + assert_eq!(expected_node_tx.lock_time.0, 0); // Must be an HTLC-Success + assert_eq!(expected_node_tx.input[0].witness.len(), 5); // Must be an HTLC-Success - check_spends!(node_txn[0], tx); + check_spends!(expected_node_tx, tx); } #[test] @@ -8852,7 +8869,7 @@ fn do_test_onchain_htlc_settlement_after_close(broadcast_alice: bool, go_onchain assert_eq!(bob_txn.len(), 1); check_spends!(bob_txn[0], txn_to_broadcast[0]); } else { - assert_eq!(bob_txn.len(), 2); + assert!(bob_txn.len() >= 2); check_spends!(bob_txn[0], chan_ab.3); } } @@ -8874,9 +8891,17 @@ fn do_test_onchain_htlc_settlement_after_close(broadcast_alice: bool, go_onchain check_spends!(bob_txn[0], txn_to_broadcast[0]); assert_eq!(bob_txn[0].input[0].witness.last().unwrap().len(), script_weight); } else { - assert_eq!(bob_txn.len(), 2); - check_spends!(bob_txn[1], txn_to_broadcast[0]); - assert_eq!(bob_txn[1].input[0].witness.last().unwrap().len(), script_weight); + assert!(bob_txn.len() >= 2); + // Multiple tx may be broadcasted here, so we loop to find the one matching the expected txid. + let mut expected_node_tx = &bob_txn[0]; + for node_tx in bob_txn.iter() { + if node_tx.input[0].previous_output.txid == txn_to_broadcast[0].txid() { + expected_node_tx = node_tx; + break; + } + } + check_spends!(expected_node_tx, txn_to_broadcast[0]); + assert_eq!(expected_node_tx.input[0].witness.last().unwrap().len(), script_weight); } } } @@ -9351,9 +9376,20 @@ fn do_test_tx_confirmed_skipping_blocks_immediate_broadcast(test_height_before_t } else { // We should broadcast an HTLC transaction spending our funding transaction first let spending_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); - assert_eq!(spending_txn.len(), 2); - assert_eq!(spending_txn[0].txid(), node_txn[0].txid()); - check_spends!(spending_txn[1], node_txn[0]); + assert!(spending_txn.len() >= 2); + // Multiple tx may be broadcasted here, so we loop to find the one matching the expected txid. + let mut spending_tx = &spending_txn[0]; + let mut spending_tx_spends = &spending_txn[1]; + for node_tx in spending_txn.iter() { + if node_tx.txid() == node_txn[0].txid() { + spending_tx = node_tx; + } + if node_tx.input[0].previous_output.txid == node_txn[0].txid() { + spending_tx_spends = node_tx; + } + } + assert_eq!(spending_tx.txid(), node_txn[0].txid()); + check_spends!(spending_tx_spends, node_txn[0]); // We should also generate a SpendableOutputs event with the to_self output (as its // timelock is up). let descriptor_spend_txn = check_spendable_outputs!(nodes[1], node_cfgs[1].keys_manager); @@ -9363,7 +9399,7 @@ fn do_test_tx_confirmed_skipping_blocks_immediate_broadcast(test_height_before_t // should immediately fail-backwards the HTLC to the previous hop, without waiting for an // additional block built on top of the current chain. nodes[1].chain_monitor.chain_monitor.transactions_confirmed( - &nodes[1].get_block_header(conf_height + 1), &[(0, &spending_txn[1])], conf_height + 1); + &nodes[1].get_block_header(conf_height + 1), &[(0, &spending_tx_spends)], conf_height + 1); expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], vec![HTLCDestination::NextHopChannel { node_id: Some(nodes[2].node.get_our_node_id()), channel_id: channel_id }]); check_added_monitors!(nodes[1], 1); diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index cb78cda714f..f7cb32b1a69 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -1673,8 +1673,16 @@ fn do_test_restored_packages_retry(check_old_monitor_retries_after_upgrade: bool connect_blocks(&nodes[0], TEST_FINAL_CLTV); let htlc_timeout_tx = { let mut txn = nodes[0].tx_broadcaster.txn_broadcast(); - assert_eq!(txn.len(), 1); - check_spends!(txn[0], commitment_tx); + assert!(txn.len() >= 1); + // Multiple tx may be broadcasted here, so we loop to find the one matching the expected txid. + let mut expected_node_tx = &txn[0]; + for node_tx in txn.iter() { + if node_tx.input[0].previous_output.txid == commitment_tx.txid() { + expected_node_tx = node_tx; + break; + } + } + check_spends!(expected_node_tx, commitment_tx); txn.pop().unwrap() }; diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 19f55f4f2d3..24ec0b3a7b4 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -627,23 +627,44 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) { mine_transaction(&nodes[1], &as_commitment_tx); let bs_htlc_claim_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); - assert_eq!(bs_htlc_claim_txn.len(), 1); - check_spends!(bs_htlc_claim_txn[0], as_commitment_tx); + assert!(bs_htlc_claim_txn.len() >= 1); + // Multiple tx may be broadcasted here, so we loop to find the one matching the expected txid. + let mut expected_node_tx = &bs_htlc_claim_txn[0]; + for node_tx in bs_htlc_claim_txn.iter() { + if node_tx.input[0].previous_output.txid == as_commitment_tx.txid() { + expected_node_tx = node_tx; + break; + } + } + check_spends!(expected_node_tx, as_commitment_tx); if !confirm_before_reload { mine_transaction(&nodes[0], &as_commitment_tx); } - mine_transaction(&nodes[0], &bs_htlc_claim_txn[0]); + mine_transaction(&nodes[0], &expected_node_tx); expect_payment_sent(&nodes[0], payment_preimage_1, None, true, false); connect_blocks(&nodes[0], TEST_FINAL_CLTV*4 + 20); - let (first_htlc_timeout_tx, second_htlc_timeout_tx) = { - let mut txn = nodes[0].tx_broadcaster.unique_txn_broadcast(); - assert_eq!(txn.len(), 2); - (txn.remove(0), txn.remove(0)) + let mut txn = nodes[0].tx_broadcaster.unique_txn_broadcast(); + assert!(txn.len() >= 2); + // Multiple tx may be broadcasted here, so we loop to find the one matching the expected txid. + let mut first_htlc_timeout_tx = &txn[0]; + let mut index = 0; + for n in index..txn.len() { + if txn[n].input[0].previous_output.txid == as_commitment_tx.txid() { + first_htlc_timeout_tx = &txn[n]; + break; + } + } + + let mut second_htlc_timeout_tx = &txn[0]; + for n in index..txn.len() { + if txn[n].input[0].previous_output.txid == as_commitment_tx.txid() { + second_htlc_timeout_tx = &txn[n]; + } }; check_spends!(first_htlc_timeout_tx, as_commitment_tx); check_spends!(second_htlc_timeout_tx, as_commitment_tx); - if first_htlc_timeout_tx.input[0].previous_output == bs_htlc_claim_txn[0].input[0].previous_output { + if first_htlc_timeout_tx.input[0].previous_output == expected_node_tx.input[0].previous_output { confirm_transaction(&nodes[0], &second_htlc_timeout_tx); } else { confirm_transaction(&nodes[0], &first_htlc_timeout_tx); @@ -800,13 +821,21 @@ fn do_test_completed_payment_not_retryable_on_reload(use_dust: bool) { connect_blocks(&nodes[0], TEST_FINAL_CLTV + (MIN_CLTV_EXPIRY_DELTA as u32)); connect_blocks(&nodes[1], TEST_FINAL_CLTV + (MIN_CLTV_EXPIRY_DELTA as u32)); let as_htlc_timeout = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); - check_spends!(as_htlc_timeout[0], bs_commitment_tx[0]); - assert_eq!(as_htlc_timeout.len(), 1); + assert!(as_htlc_timeout.len() >= 1); + // Multiple tx may be broadcasted here, so we loop to find the one matching the expected txid. + let mut expected_node_tx = &as_htlc_timeout[0]; + for node_tx in as_htlc_timeout.iter() { + if node_tx.input[0].previous_output.txid == bs_commitment_tx[0].txid() { + expected_node_tx = node_tx; + break; + } + } + check_spends!(expected_node_tx, bs_commitment_tx[0]); - mine_transaction(&nodes[0], &as_htlc_timeout[0]); + mine_transaction(&nodes[0], &expected_node_tx); // nodes[0] may rebroadcast (or RBF-bump) its HTLC-Timeout, so wipe the announced set. nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().clear(); - mine_transaction(&nodes[1], &as_htlc_timeout[0]); + mine_transaction(&nodes[1], &expected_node_tx); } // Create a new channel on which to retry the payment before we fail the payment via the From 3f13b325cad6d7040422ef9ea5fcf0d7004802ba Mon Sep 17 00:00:00 2001 From: Rachel Malonson Date: Wed, 4 Oct 2023 18:58:23 -0700 Subject: [PATCH 3/3] Refactor block_confirmed --- lightning/src/chain/channelmonitor.rs | 27 ++++----------------------- 1 file changed, 4 insertions(+), 23 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index b4551f96f5d..69805306df8 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -3493,29 +3493,10 @@ impl ChannelMonitorImpl { let should_broadcast = self.should_broadcast_holder_commitment_txn(logger); if should_broadcast { - let funding_outp = HolderFundingOutput::build(self.funding_redeemscript.clone(), self.channel_value_satoshis, self.onchain_tx_handler.channel_type_features().clone()); - let commitment_package = PackageTemplate::build_package(self.funding_info.0.txid.clone(), self.funding_info.0.index as u32, PackageSolvingData::HolderFundingOutput(funding_outp), self.best_block.height(), self.best_block.height()); - claimable_outpoints.push(commitment_package); - self.pending_monitor_events.push(MonitorEvent::HolderForceClosed(self.funding_info.0)); - // Although we aren't signing the transaction directly here, the transaction will be signed - // in the claim that is queued to OnchainTxHandler. We set holder_tx_signed here to reject - // new channel updates. - self.holder_tx_signed = true; - // We can't broadcast our HTLC transactions while the commitment transaction is - // unconfirmed. We'll delay doing so until we detect the confirmed commitment in - // `transactions_confirmed`. - if !self.onchain_tx_handler.channel_type_features().supports_anchors_zero_fee_htlc_tx() { - // Because we're broadcasting a commitment transaction, we should construct the package - // assuming it gets confirmed in the next block. Sadly, we have code which considers - // "not yet confirmed" things as discardable, so we cannot do that here. - let (mut new_outpoints, _) = self.get_broadcasted_holder_claims(&self.current_holder_commitment_tx, self.best_block.height()); - let unsigned_commitment_tx = self.onchain_tx_handler.get_unsigned_holder_commitment_tx(); - let new_outputs = self.get_broadcasted_holder_watch_outputs(&self.current_holder_commitment_tx, &unsigned_commitment_tx); - if !new_outputs.is_empty() { - watch_outputs.push((self.current_holder_commitment_tx.txid.clone(), new_outputs)); - } - claimable_outpoints.append(&mut new_outpoints); - } + let (mut new_claimable_outpoints, mut new_watch_outputs) = self.generate_claimable_outpoints_and_watch_outputs(); + claimable_outpoints.append(&mut new_claimable_outpoints); + watch_outputs.append(&mut new_watch_outputs); + } // Find which on-chain events have reached their confirmation threshold.