From 19de4353d52bca8021ba3e1b8f4b036d5f5a8648 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 11 Jul 2023 15:14:01 -0700 Subject: [PATCH 1/8] Move feerate helpers to chain module We plan to use these outside of the `bump_transaction` module in the next commit, and they really should belong in the same module as `FeeEstimator`. --- lightning/src/chain/chaininterface.rs | 9 +++++++++ lightning/src/events/bump_transaction.rs | 11 +---------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/lightning/src/chain/chaininterface.rs b/lightning/src/chain/chaininterface.rs index d875dcce3e1..54cd3381209 100644 --- a/lightning/src/chain/chaininterface.rs +++ b/lightning/src/chain/chaininterface.rs @@ -14,9 +14,18 @@ //! disconnections, transaction broadcasting, and feerate information requests. use core::{cmp, ops::Deref}; +use core::convert::TryInto; use bitcoin::blockdata::transaction::Transaction; +// TODO: Define typed abstraction over feerates to handle their conversions. +pub(crate) fn compute_feerate_sat_per_1000_weight(fee_sat: u64, weight: u64) -> u32 { + (fee_sat * 1000 / weight).try_into().unwrap_or(u32::max_value()) +} +pub(crate) const fn fee_for_weight(feerate_sat_per_1000_weight: u32, weight: u64) -> u64 { + ((feerate_sat_per_1000_weight as u64 * weight) + 1000 - 1) / 1000 +} + /// An interface to send a transaction to the Bitcoin network. pub trait BroadcasterInterface { /// Sends a list of transactions out to (hopefully) be mined. diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs index 1f28bcfb938..dbe539505d6 100644 --- a/lightning/src/events/bump_transaction.rs +++ b/lightning/src/events/bump_transaction.rs @@ -12,10 +12,9 @@ //! [`Event`]: crate::events::Event use alloc::collections::BTreeMap; -use core::convert::TryInto; use core::ops::Deref; -use crate::chain::chaininterface::BroadcasterInterface; +use crate::chain::chaininterface::{BroadcasterInterface, compute_feerate_sat_per_1000_weight, fee_for_weight}; use crate::chain::ClaimId; use crate::io_extras::sink; use crate::ln::channel::ANCHOR_OUTPUT_VALUE_SATOSHI; @@ -44,14 +43,6 @@ const BASE_INPUT_SIZE: u64 = 32 /* txid */ + 4 /* vout */ + 4 /* sequence */; const BASE_INPUT_WEIGHT: u64 = BASE_INPUT_SIZE * WITNESS_SCALE_FACTOR as u64; -// TODO: Define typed abstraction over feerates to handle their conversions. -fn compute_feerate_sat_per_1000_weight(fee_sat: u64, weight: u64) -> u32 { - (fee_sat * 1000 / weight).try_into().unwrap_or(u32::max_value()) -} -const fn fee_for_weight(feerate_sat_per_1000_weight: u32, weight: u64) -> u64 { - ((feerate_sat_per_1000_weight as u64 * weight) + 1000 - 1) / 1000 -} - /// The parameters required to derive a channel signer via [`SignerProvider`]. #[derive(Clone, Debug, PartialEq, Eq)] pub struct ChannelDerivationParameters { From 990c5000999d3497f13b802f7fd4dce014704a08 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 11 Jul 2023 15:16:16 -0700 Subject: [PATCH 2/8] Avoid yielding ChannelClose bump events with sufficient feerate There's no need to yield such an event when the commitment transaction already meets the target feerate on its own, so we can simply broadcast it without an anchor child transaction. This may be a common occurrence until we are less aggressive about feerate updates. --- lightning/src/chain/onchaintx.rs | 24 ++++++++++++++++++++---- lightning/src/chain/package.rs | 2 +- lightning/src/events/bump_transaction.rs | 12 ------------ lightning/src/ln/monitor_tests.rs | 2 ++ 4 files changed, 23 insertions(+), 17 deletions(-) diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 6b0c7485610..171caf7006e 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -22,6 +22,7 @@ use bitcoin::hash_types::{Txid, BlockHash}; use bitcoin::secp256k1::{Secp256k1, ecdsa::Signature}; use bitcoin::secp256k1; +use crate::chain::chaininterface::compute_feerate_sat_per_1000_weight; use crate::sign::{ChannelSigner, EntropySource, SignerProvider}; use crate::ln::msgs::DecodeError; use crate::ln::PaymentPreimage; @@ -623,9 +624,24 @@ impl OnchainTxHandler return inputs.find_map(|input| match input { // Commitment inputs with anchors support are the only untractable inputs supported // thus far that require external funding. - PackageSolvingData::HolderFundingOutput(..) => { + PackageSolvingData::HolderFundingOutput(output) => { debug_assert_eq!(tx.txid(), self.holder_commitment.trust().txid(), "Holder commitment transaction mismatch"); + + let conf_target = ConfirmationTarget::HighPriority; + let package_target_feerate_sat_per_1000_weight = cached_request + .compute_package_feerate(fee_estimator, conf_target, force_feerate_bump); + if let Some(input_amount_sat) = output.funding_amount { + let fee_sat = input_amount_sat - tx.output.iter().map(|output| output.value).sum::(); + if compute_feerate_sat_per_1000_weight(fee_sat, tx.weight() as u64) >= + package_target_feerate_sat_per_1000_weight + { + log_debug!(logger, "Commitment transaction {} already meets required feerate {} sat/kW", + tx.txid(), package_target_feerate_sat_per_1000_weight); + return Some((new_timer, 0, OnchainClaim::Tx(tx.clone()))); + } + } + // We'll locate an anchor output we can spend within the commitment transaction. let funding_pubkey = &self.channel_transaction_parameters.holder_pubkeys.funding_pubkey; match chan_utils::get_anchor_output(&tx, funding_pubkey) { @@ -633,9 +649,6 @@ impl OnchainTxHandler Some((idx, _)) => { // TODO: Use a lower confirmation target when both our and the // counterparty's latest commitment don't have any HTLCs present. - let conf_target = ConfirmationTarget::HighPriority; - let package_target_feerate_sat_per_1000_weight = cached_request - .compute_package_feerate(fee_estimator, conf_target, force_feerate_bump); Some(( new_timer, package_target_feerate_sat_per_1000_weight as u64, @@ -739,6 +752,9 @@ impl OnchainTxHandler ) { req.set_timer(new_timer); req.set_feerate(new_feerate); + // Once a pending claim has an id assigned, it remains fixed until the claim is + // satisfied, regardless of whether the claim switches between different variants of + // `OnchainClaim`. let claim_id = match claim { OnchainClaim::Tx(tx) => { log_info!(logger, "Broadcasting onchain {}", log_tx!(tx)); diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index 9e61cbc4598..b66a2f70d33 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -429,7 +429,7 @@ impl Readable for HolderHTLCOutput { #[derive(Clone, PartialEq, Eq)] pub(crate) struct HolderFundingOutput { funding_redeemscript: Script, - funding_amount: Option, + pub(crate) funding_amount: Option, channel_type_features: ChannelTypeFeatures, } diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs index dbe539505d6..ffd9b22834d 100644 --- a/lightning/src/events/bump_transaction.rs +++ b/lightning/src/events/bump_transaction.rs @@ -700,18 +700,6 @@ where &self, claim_id: ClaimId, package_target_feerate_sat_per_1000_weight: u32, commitment_tx: &Transaction, commitment_tx_fee_sat: u64, anchor_descriptor: &AnchorDescriptor, ) -> Result<(), ()> { - // Compute the feerate the anchor transaction must meet to meet the overall feerate for the - // package (commitment + anchor transactions). - let commitment_tx_sat_per_1000_weight: u32 = compute_feerate_sat_per_1000_weight( - commitment_tx_fee_sat, commitment_tx.weight() as u64, - ); - if commitment_tx_sat_per_1000_weight >= package_target_feerate_sat_per_1000_weight { - // If the commitment transaction already has a feerate high enough on its own, broadcast - // it as is without a child. - self.broadcaster.broadcast_transactions(&[&commitment_tx]); - return Ok(()); - } - let mut anchor_tx = self.build_anchor_tx( claim_id, package_target_feerate_sat_per_1000_weight, commitment_tx, anchor_descriptor, )?; diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index 41ecd9da0fb..22b5fa7ce34 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -1878,6 +1878,7 @@ fn test_yield_anchors_events() { assert!(nodes[0].node.get_and_clear_pending_events().is_empty()); + *nodes[0].fee_estimator.sat_per_kw.lock().unwrap() *= 2; connect_blocks(&nodes[0], TEST_FINAL_CLTV + LATENCY_GRACE_PERIOD_BLOCKS + 1); check_closed_broadcast!(&nodes[0], true); assert!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().is_empty()); @@ -2054,6 +2055,7 @@ fn test_anchors_aggregated_revoked_htlc_tx() { // Bob force closes by restarting with the outdated state, prompting the ChannelMonitors to // broadcast the latest commitment transaction known to them, which in our case is the one with // the HTLCs still pending. + *nodes[1].fee_estimator.sat_per_kw.lock().unwrap() *= 2; nodes[1].node.timer_tick_occurred(); check_added_monitors(&nodes[1], 2); check_closed_event!(&nodes[1], 2, ClosureReason::OutdatedChannelManager); From 9088dae7daab47c46ad9b8447c376eceade7c7c4 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 11 Jul 2023 15:30:51 -0700 Subject: [PATCH 3/8] Consider existing commitment transaction feerate when bumping With anchors, we've yet to change the frequency or aggressiveness of feerate updates, so it's likely that commitment transactions have a good enough feerate to confirm on its own. In any case, when producing a child anchor transaction, we should already take into account the fees paid by the commitment transaction itself, allowing the user to save some satoshis. Unfortunately, in its current form, this will still result in overpaying by a small margin at the expense of making the coin selection API more complex. --- lightning/src/events/bump_transaction.rs | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs index ffd9b22834d..f9d956ec297 100644 --- a/lightning/src/events/bump_transaction.rs +++ b/lightning/src/events/bump_transaction.rs @@ -14,7 +14,7 @@ use alloc::collections::BTreeMap; use core::ops::Deref; -use crate::chain::chaininterface::{BroadcasterInterface, compute_feerate_sat_per_1000_weight, fee_for_weight}; +use crate::chain::chaininterface::{BroadcasterInterface, compute_feerate_sat_per_1000_weight, fee_for_weight, FEERATE_FLOOR_SATS_PER_KW}; use crate::chain::ClaimId; use crate::io_extras::sink; use crate::ln::channel::ANCHOR_OUTPUT_VALUE_SATOSHI; @@ -700,8 +700,21 @@ where &self, claim_id: ClaimId, package_target_feerate_sat_per_1000_weight: u32, commitment_tx: &Transaction, commitment_tx_fee_sat: u64, anchor_descriptor: &AnchorDescriptor, ) -> Result<(), ()> { + // Our commitment transaction already has fees allocated to it, so we should take them into + // account. We compute its feerate and subtract it from the package target, using the result + // as the target feerate for our anchor transaction. Unfortunately, this results in users + // overpaying by a small margin since we don't yet know the anchor transaction size, and + // avoiding the small overpayment only makes our API even more complex. + let commitment_tx_sat_per_1000_weight: u32 = compute_feerate_sat_per_1000_weight( + commitment_tx_fee_sat, commitment_tx.weight() as u64, + ); + let anchor_target_feerate_sat_per_1000_weight = core::cmp::max( + package_target_feerate_sat_per_1000_weight - commitment_tx_sat_per_1000_weight, + FEERATE_FLOOR_SATS_PER_KW, + ); + let mut anchor_tx = self.build_anchor_tx( - claim_id, package_target_feerate_sat_per_1000_weight, commitment_tx, anchor_descriptor, + claim_id, anchor_target_feerate_sat_per_1000_weight, commitment_tx, anchor_descriptor, )?; debug_assert_eq!(anchor_tx.output.len(), 1); From eac1bc18e37ddcea304f26d4d797381e717708ef Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 11 Jul 2023 15:17:34 -0700 Subject: [PATCH 4/8] Add debug assertions for weight estimates of bump transactions This ensures our estimates are correct by never underestimating and only allowing overestimations by a margin of 1%. --- lightning/src/events/bump_transaction.rs | 101 ++++++++++++----------- 1 file changed, 53 insertions(+), 48 deletions(-) diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs index f9d956ec297..dbe1b734e56 100644 --- a/lightning/src/events/bump_transaction.rs +++ b/lightning/src/events/bump_transaction.rs @@ -668,31 +668,6 @@ where } } - /// Returns an unsigned transaction spending an anchor output of the commitment transaction, and - /// any additional UTXOs sourced, to bump the commitment transaction's fee. - fn build_anchor_tx( - &self, claim_id: ClaimId, target_feerate_sat_per_1000_weight: u32, - commitment_tx: &Transaction, anchor_descriptor: &AnchorDescriptor, - ) -> Result { - let must_spend = vec![Input { - outpoint: anchor_descriptor.outpoint, - previous_utxo: anchor_descriptor.previous_utxo(), - satisfaction_weight: commitment_tx.weight() as u64 + ANCHOR_INPUT_WITNESS_WEIGHT + EMPTY_SCRIPT_SIG_WEIGHT, - }]; - let coin_selection = self.utxo_source.select_confirmed_utxos( - claim_id, &must_spend, &[], target_feerate_sat_per_1000_weight, - )?; - - let mut tx = Transaction { - version: 2, - lock_time: PackedLockTime::ZERO, // TODO: Use next best height. - input: vec![anchor_descriptor.unsigned_tx_input()], - output: vec![], - }; - self.process_coin_selection(&mut tx, coin_selection); - Ok(tx) - } - /// Handles a [`BumpTransactionEvent::ChannelClose`] event variant by producing a fully-signed /// transaction spending an anchor output of the commitment transaction to bump its fee and /// broadcasts them to the network as a package. @@ -713,27 +688,55 @@ where FEERATE_FLOOR_SATS_PER_KW, ); - let mut anchor_tx = self.build_anchor_tx( - claim_id, anchor_target_feerate_sat_per_1000_weight, commitment_tx, anchor_descriptor, + let must_spend = vec![Input { + outpoint: anchor_descriptor.outpoint, + previous_utxo: anchor_descriptor.previous_utxo(), + satisfaction_weight: commitment_tx.weight() as u64 + ANCHOR_INPUT_WITNESS_WEIGHT + EMPTY_SCRIPT_SIG_WEIGHT, + }]; + let coin_selection = self.utxo_source.select_confirmed_utxos( + claim_id, &must_spend, &[], anchor_target_feerate_sat_per_1000_weight, )?; + + let mut anchor_tx = Transaction { + version: 2, + lock_time: PackedLockTime::ZERO, // TODO: Use next best height. + input: vec![anchor_descriptor.unsigned_tx_input()], + output: vec![], + }; + #[cfg(debug_assertions)] + let total_satisfaction_weight = + coin_selection.confirmed_utxos.iter().map(|utxo| utxo.satisfaction_weight).sum::() + + ANCHOR_INPUT_WITNESS_WEIGHT + EMPTY_SCRIPT_SIG_WEIGHT; + + self.process_coin_selection(&mut anchor_tx, coin_selection); + debug_assert_eq!(anchor_tx.output.len(), 1); + #[cfg(debug_assertions)] + let unsigned_tx_weight = anchor_tx.weight() as u64 - (anchor_tx.input.len() as u64 * EMPTY_SCRIPT_SIG_WEIGHT); self.utxo_source.sign_tx(&mut anchor_tx)?; let signer = anchor_descriptor.derive_channel_signer(&self.signer_provider); let anchor_sig = signer.sign_holder_anchor_input(&anchor_tx, 0, &self.secp)?; anchor_tx.input[0].witness = anchor_descriptor.tx_input_witness(&anchor_sig); + #[cfg(debug_assertions)] { + let signed_tx_weight = anchor_tx.weight() as u64; + let expected_signed_tx_weight = unsigned_tx_weight + total_satisfaction_weight; + // Our estimate should be within a 1% error margin of the actual weight. + assert!(expected_signed_tx_weight - (expected_signed_tx_weight / 100) <= signed_tx_weight); + } + self.broadcaster.broadcast_transactions(&[&commitment_tx, &anchor_tx]); Ok(()) } - /// Returns an unsigned, fee-bumped HTLC transaction, along with the set of signers required to - /// fulfill the witness for each HTLC input within it. - fn build_htlc_tx( + /// Handles a [`BumpTransactionEvent::HTLCResolution`] event variant by producing a + /// fully-signed, fee-bumped HTLC transaction that is broadcast to the network. + fn handle_htlc_resolution( &self, claim_id: ClaimId, target_feerate_sat_per_1000_weight: u32, htlc_descriptors: &[HTLCDescriptor], tx_lock_time: PackedLockTime, - ) -> Result { - let mut tx = Transaction { + ) -> Result<(), ()> { + let mut htlc_tx = Transaction { version: 2, lock_time: tx_lock_time, input: vec![], @@ -751,27 +754,22 @@ where HTLC_TIMEOUT_INPUT_ANCHOR_WITNESS_WEIGHT }, }); - tx.input.push(htlc_input); + htlc_tx.input.push(htlc_input); let htlc_output = htlc_descriptor.tx_output(&self.secp); - tx.output.push(htlc_output); + htlc_tx.output.push(htlc_output); } let coin_selection = self.utxo_source.select_confirmed_utxos( - claim_id, &must_spend, &tx.output, target_feerate_sat_per_1000_weight, + claim_id, &must_spend, &htlc_tx.output, target_feerate_sat_per_1000_weight, )?; - self.process_coin_selection(&mut tx, coin_selection); - Ok(tx) - } + #[cfg(debug_assertions)] + let total_satisfaction_weight = + coin_selection.confirmed_utxos.iter().map(|utxo| utxo.satisfaction_weight).sum::() + + must_spend.iter().map(|input| input.satisfaction_weight).sum::(); + self.process_coin_selection(&mut htlc_tx, coin_selection); - /// Handles a [`BumpTransactionEvent::HTLCResolution`] event variant by producing a - /// fully-signed, fee-bumped HTLC transaction that is broadcast to the network. - fn handle_htlc_resolution( - &self, claim_id: ClaimId, target_feerate_sat_per_1000_weight: u32, - htlc_descriptors: &[HTLCDescriptor], tx_lock_time: PackedLockTime, - ) -> Result<(), ()> { - let mut htlc_tx = self.build_htlc_tx( - claim_id, target_feerate_sat_per_1000_weight, htlc_descriptors, tx_lock_time, - )?; + #[cfg(debug_assertions)] + let unsigned_tx_weight = htlc_tx.weight() as u64 - (htlc_tx.input.len() as u64 * EMPTY_SCRIPT_SIG_WEIGHT); self.utxo_source.sign_tx(&mut htlc_tx)?; let mut signers = BTreeMap::new(); @@ -783,6 +781,13 @@ where htlc_tx.input[idx].witness = htlc_descriptor.tx_input_witness(&htlc_sig, &witness_script); } + #[cfg(debug_assertions)] { + let signed_tx_weight = htlc_tx.weight() as u64; + let expected_signed_tx_weight = unsigned_tx_weight + total_satisfaction_weight; + // Our estimate should be within a 1% error margin of the actual weight. + assert!(expected_signed_tx_weight - (expected_signed_tx_weight / 100) <= signed_tx_weight); + } + self.broadcaster.broadcast_transactions(&[&htlc_tx]); Ok(()) } @@ -792,7 +797,7 @@ where match event { BumpTransactionEvent::ChannelClose { claim_id, package_target_feerate_sat_per_1000_weight, commitment_tx, - anchor_descriptor, commitment_tx_fee_satoshis, .. + commitment_tx_fee_satoshis, anchor_descriptor, .. } => { if let Err(_) = self.handle_channel_close( *claim_id, *package_target_feerate_sat_per_1000_weight, commitment_tx, From c18013c2b66ca7dd743ec2b2c7d379d42ee454a7 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Fri, 14 Jul 2023 14:44:00 -0700 Subject: [PATCH 5/8] Add log_iter utility macro This is a useful primitive to have that formats the contents of the iterator as a comma-separated list. --- lightning/src/util/logger.rs | 23 +++++++++++++++++++++++ lightning/src/util/macro_logger.rs | 6 ++++++ 2 files changed, 29 insertions(+) diff --git a/lightning/src/util/logger.rs b/lightning/src/util/logger.rs index aac83f42a3c..722a81f7ab6 100644 --- a/lightning/src/util/logger.rs +++ b/lightning/src/util/logger.rs @@ -169,6 +169,29 @@ impl<'a> core::fmt::Display for DebugBytes<'a> { } } +/// Wrapper for logging `Iterator`s. +/// +/// This is not exported to bindings users as fmt can't be used in C +#[doc(hidden)] +pub struct DebugIter + Clone>(pub core::cell::RefCell); +impl + Clone> fmt::Display for DebugIter { + fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> { + use core::ops::DerefMut; + write!(f, "[")?; + let iter_ref = self.0.clone(); + let mut iter = iter_ref.borrow_mut(); + for item in iter.deref_mut() { + write!(f, "{}", item)?; + break; + } + for item in iter.deref_mut() { + write!(f, ", {}", item)?; + } + write!(f, "]")?; + Ok(()) + } +} + #[cfg(test)] mod tests { use crate::util::logger::{Logger, Level}; diff --git a/lightning/src/util/macro_logger.rs b/lightning/src/util/macro_logger.rs index 8742e8e84d0..4703bcdb32a 100644 --- a/lightning/src/util/macro_logger.rs +++ b/lightning/src/util/macro_logger.rs @@ -17,6 +17,12 @@ use crate::routing::router::Route; use crate::ln::chan_utils::HTLCClaim; use crate::util::logger::DebugBytes; +macro_rules! log_iter { + ($obj: expr) => { + $crate::util::logger::DebugIter(core::cell::RefCell::new($obj)) + } +} + /// Logs a pubkey in hex format. #[macro_export] macro_rules! log_pubkey { From 964b4939b16f5d768f68ba4ebf2f43e544970d76 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 11 Jul 2023 16:43:10 -0700 Subject: [PATCH 6/8] Improve logging in BumpTransactionEventHandler paths --- lightning/src/events/bump_transaction.rs | 66 ++++++++++++++++++++---- 1 file changed, 57 insertions(+), 9 deletions(-) diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs index dbe1b734e56..bf56a84ca46 100644 --- a/lightning/src/events/bump_transaction.rs +++ b/lightning/src/events/bump_transaction.rs @@ -140,6 +140,15 @@ pub struct HTLCDescriptor { } impl HTLCDescriptor { + /// Returns the outpoint of the HTLC output in the commitment transaction. This is the outpoint + /// being spent by the HTLC input in the HTLC transaction. + pub fn outpoint(&self) -> OutPoint { + OutPoint { + txid: self.commitment_txid, + vout: self.htlc.transaction_output_index.unwrap(), + } + } + /// Returns the UTXO to be spent by the HTLC input, which can be obtained via /// [`Self::unsigned_tx_input`]. pub fn previous_utxo(&self, secp: &Secp256k1) -> TxOut { @@ -480,19 +489,28 @@ pub trait WalletSource { /// A wrapper over [`WalletSource`] that implements [`CoinSelection`] by preferring UTXOs that would /// avoid conflicting double spends. If not enough UTXOs are available to do so, conflicting double /// spends may happen. -pub struct Wallet where W::Target: WalletSource { +pub struct Wallet +where + W::Target: WalletSource, + L::Target: Logger +{ source: W, + logger: L, // TODO: Do we care about cleaning this up once the UTXOs have a confirmed spend? We can do so // by checking whether any UTXOs that exist in the map are no longer returned in // `list_confirmed_utxos`. locked_utxos: Mutex>, } -impl Wallet where W::Target: WalletSource { +impl Wallet +where + W::Target: WalletSource, + L::Target: Logger +{ /// Returns a new instance backed by the given [`WalletSource`] that serves as an implementation /// of [`CoinSelectionSource`]. - pub fn new(source: W) -> Self { - Self { source, locked_utxos: Mutex::new(HashMap::new()) } + pub fn new(source: W, logger: L) -> Self { + Self { source, logger, locked_utxos: Mutex::new(HashMap::new()) } } /// Performs coin selection on the set of UTXOs obtained from @@ -512,6 +530,7 @@ impl Wallet where W::Target: WalletSource { let mut eligible_utxos = utxos.iter().filter_map(|utxo| { if let Some(utxo_claim_id) = locked_utxos.get(&utxo.outpoint) { if *utxo_claim_id != claim_id && !force_conflicting_utxo_spend { + log_trace!(self.logger, "Skipping UTXO {} to prevent conflicting spend", utxo.outpoint); return None; } } @@ -526,6 +545,7 @@ impl Wallet where W::Target: WalletSource { if should_spend { Some((utxo, fee_to_spend_utxo)) } else { + log_trace!(self.logger, "Skipping UTXO {} due to dust proximity after spend", utxo.outpoint); None } }).collect::>(); @@ -543,6 +563,8 @@ impl Wallet where W::Target: WalletSource { selected_utxos.push(utxo.clone()); } if selected_amount < target_amount_sat + total_fees { + log_debug!(self.logger, "Insufficient funds to meet target feerate {} sat/kW", + target_feerate_sat_per_1000_weight); return Err(()); } for utxo in &selected_utxos { @@ -559,6 +581,7 @@ impl Wallet where W::Target: WalletSource { ); let change_output_amount = remaining_amount.saturating_sub(change_output_fee); let change_output = if change_output_amount < change_script.dust_value().to_sat() { + log_debug!(self.logger, "Coin selection attempt did not yield change output"); None } else { Some(TxOut { script_pubkey: change_script, value: change_output_amount }) @@ -571,7 +594,11 @@ impl Wallet where W::Target: WalletSource { } } -impl CoinSelectionSource for Wallet where W::Target: WalletSource { +impl CoinSelectionSource for Wallet +where + W::Target: WalletSource, + L::Target: Logger +{ fn select_confirmed_utxos( &self, claim_id: ClaimId, must_spend: &[Input], must_pay_to: &[TxOut], target_feerate_sat_per_1000_weight: u32, @@ -589,6 +616,8 @@ impl CoinSelectionSource for Wallet where W::Target: WalletSource { ((BASE_TX_SIZE + total_output_size) * WITNESS_SCALE_FACTOR as u64); let target_amount_sat = must_pay_to.iter().map(|output| output.value).sum(); let do_coin_selection = |force_conflicting_utxo_spend: bool, tolerate_high_network_feerates: bool| { + log_debug!(self.logger, "Attempting coin selection targeting {} sat/kW (force_conflicting_utxo_spend = {}, tolerate_high_network_feerates = {})", + target_feerate_sat_per_1000_weight, force_conflicting_utxo_spend, tolerate_high_network_feerates); self.select_confirmed_utxos_internal( &utxos, claim_id, force_conflicting_utxo_spend, tolerate_high_network_feerates, target_feerate_sat_per_1000_weight, preexisting_tx_weight, target_amount_sat, @@ -661,6 +690,7 @@ where // match, but we still need to have at least one output in the transaction for it to be // considered standard. We choose to go with an empty OP_RETURN as it is the cheapest // way to include a dummy output. + log_debug!(self.logger, "Including dummy OP_RETURN output since an output is needed and a change output was not provided"); tx.output.push(TxOut { value: 0, script_pubkey: Script::new_op_return(&[]), @@ -688,6 +718,8 @@ where FEERATE_FLOOR_SATS_PER_KW, ); + log_debug!(self.logger, "Peforming coin selection for anchor transaction targeting {} sat/kW", + anchor_target_feerate_sat_per_1000_weight); let must_spend = vec![Input { outpoint: anchor_descriptor.outpoint, previous_utxo: anchor_descriptor.previous_utxo(), @@ -709,11 +741,13 @@ where ANCHOR_INPUT_WITNESS_WEIGHT + EMPTY_SCRIPT_SIG_WEIGHT; self.process_coin_selection(&mut anchor_tx, coin_selection); + let anchor_txid = anchor_tx.txid(); debug_assert_eq!(anchor_tx.output.len(), 1); #[cfg(debug_assertions)] let unsigned_tx_weight = anchor_tx.weight() as u64 - (anchor_tx.input.len() as u64 * EMPTY_SCRIPT_SIG_WEIGHT); + log_debug!(self.logger, "Signing anchor transaction {}", anchor_txid); self.utxo_source.sign_tx(&mut anchor_tx)?; let signer = anchor_descriptor.derive_channel_signer(&self.signer_provider); let anchor_sig = signer.sign_holder_anchor_input(&anchor_tx, 0, &self.secp)?; @@ -722,10 +756,14 @@ where #[cfg(debug_assertions)] { let signed_tx_weight = anchor_tx.weight() as u64; let expected_signed_tx_weight = unsigned_tx_weight + total_satisfaction_weight; - // Our estimate should be within a 1% error margin of the actual weight. - assert!(expected_signed_tx_weight - (expected_signed_tx_weight / 100) <= signed_tx_weight); + // Our estimate should be within a 1% error margin of the actual weight and we should + // never underestimate. + assert!(expected_signed_tx_weight >= signed_tx_weight && + expected_signed_tx_weight - (expected_signed_tx_weight / 100) <= signed_tx_weight); } + log_info!(self.logger, "Broadcasting anchor transaction {} to bump channel close with txid {}", + anchor_txid, commitment_tx.txid()); self.broadcaster.broadcast_transactions(&[&commitment_tx, &anchor_tx]); Ok(()) } @@ -759,6 +797,8 @@ where htlc_tx.output.push(htlc_output); } + log_debug!(self.logger, "Peforming coin selection for HTLC transaction targeting {} sat/kW", + target_feerate_sat_per_1000_weight); let coin_selection = self.utxo_source.select_confirmed_utxos( claim_id, &must_spend, &htlc_tx.output, target_feerate_sat_per_1000_weight, )?; @@ -771,6 +811,7 @@ where #[cfg(debug_assertions)] let unsigned_tx_weight = htlc_tx.weight() as u64 - (htlc_tx.input.len() as u64 * EMPTY_SCRIPT_SIG_WEIGHT); + log_debug!(self.logger, "Signing HTLC transaction {}", htlc_tx.txid()); self.utxo_source.sign_tx(&mut htlc_tx)?; let mut signers = BTreeMap::new(); for (idx, htlc_descriptor) in htlc_descriptors.iter().enumerate() { @@ -784,10 +825,13 @@ where #[cfg(debug_assertions)] { let signed_tx_weight = htlc_tx.weight() as u64; let expected_signed_tx_weight = unsigned_tx_weight + total_satisfaction_weight; - // Our estimate should be within a 1% error margin of the actual weight. - assert!(expected_signed_tx_weight - (expected_signed_tx_weight / 100) <= signed_tx_weight); + // Our estimate should be within a 1% error margin of the actual weight and we should + // never underestimate. + assert!(expected_signed_tx_weight >= signed_tx_weight && + expected_signed_tx_weight - (expected_signed_tx_weight / 100) <= signed_tx_weight); } + log_info!(self.logger, "Broadcasting {}", log_tx!(htlc_tx)); self.broadcaster.broadcast_transactions(&[&htlc_tx]); Ok(()) } @@ -799,6 +843,8 @@ where claim_id, package_target_feerate_sat_per_1000_weight, commitment_tx, commitment_tx_fee_satoshis, anchor_descriptor, .. } => { + log_info!(self.logger, "Handling channel close bump (claim_id = {}, commitment_txid = {})", + log_bytes!(claim_id.0), commitment_tx.txid()); if let Err(_) = self.handle_channel_close( *claim_id, *package_target_feerate_sat_per_1000_weight, commitment_tx, *commitment_tx_fee_satoshis, anchor_descriptor, @@ -810,6 +856,8 @@ where BumpTransactionEvent::HTLCResolution { claim_id, target_feerate_sat_per_1000_weight, htlc_descriptors, tx_lock_time, } => { + log_info!(self.logger, "Handling HTLC bump (claim_id = {}, htlcs_to_claim = {})", + log_bytes!(claim_id.0), log_iter!(htlc_descriptors.iter().map(|d| d.outpoint()))); if let Err(_) = self.handle_htlc_resolution( *claim_id, *target_feerate_sat_per_1000_weight, htlc_descriptors, *tx_lock_time, ) { From 38f12698a7fba7e03fcb18633ff91747da546ee0 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 11 Jul 2023 15:12:19 -0700 Subject: [PATCH 7/8] Add BumpTransactionEventHandler instance to node test harness --- lightning/src/ln/functional_test_utils.rs | 33 ++++++++++-- lightning/src/util/test_utils.rs | 65 +++++++++++++++++++++++ 2 files changed, 94 insertions(+), 4 deletions(-) diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 024690d00da..220557e4ca3 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -15,6 +15,7 @@ use crate::sign::EntropySource; use crate::chain::channelmonitor::ChannelMonitor; use crate::chain::transaction::OutPoint; use crate::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentPurpose, PaymentFailureReason}; +use crate::events::bump_transaction::{BumpTransactionEventHandler, Wallet, WalletSource}; use crate::ln::{PaymentPreimage, PaymentHash, PaymentSecret}; use crate::ln::channelmanager::{AChannelManager, ChainParameters, ChannelManager, ChannelManagerReadArgs, RAACommitmentOrder, PaymentSendFailure, RecipientOnionFields, PaymentId, MIN_CLTV_EXPIRY_DELTA}; use crate::routing::gossip::{P2PGossipSync, NetworkGraph, NetworkUpdate}; @@ -32,13 +33,11 @@ use crate::util::ser::{ReadableArgs, Writeable}; use bitcoin::blockdata::block::{Block, BlockHeader}; use bitcoin::blockdata::transaction::{Transaction, TxOut}; -use bitcoin::network::constants::Network; - use bitcoin::hash_types::BlockHash; use bitcoin::hashes::sha256::Hash as Sha256; use bitcoin::hashes::Hash as _; - -use bitcoin::secp256k1::PublicKey; +use bitcoin::network::constants::Network; +use bitcoin::secp256k1::{PublicKey, SecretKey}; use crate::io; use crate::prelude::*; @@ -289,6 +288,19 @@ fn do_connect_block<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, block: Block, sk } call_claimable_balances(node); node.node.test_process_background_events(); + + for tx in &block.txdata { + for input in &tx.input { + node.wallet_source.remove_utxo(input.previous_output); + } + let wallet_script = node.wallet_source.get_change_script().unwrap(); + for (idx, output) in tx.output.iter().enumerate() { + if output.script_pubkey == wallet_script { + let outpoint = bitcoin::OutPoint { txid: tx.txid(), vout: idx as u32 }; + node.wallet_source.add_utxo(outpoint, output.value); + } + } + } } pub fn disconnect_blocks<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, count: u32) { @@ -375,6 +387,13 @@ pub struct Node<'a, 'b: 'a, 'c: 'b> { pub blocks: Arc>>, pub connect_style: Rc>, pub override_init_features: Rc>>, + pub wallet_source: Arc, + pub bump_tx_handler: BumpTransactionEventHandler< + &'c test_utils::TestBroadcaster, + Arc, &'c test_utils::TestLogger>>, + &'b test_utils::TestKeysInterface, + &'c test_utils::TestLogger, + >, } impl<'a, 'b, 'c> Node<'a, 'b, 'c> { pub fn best_block_hash(&self) -> BlockHash { @@ -2622,6 +2641,7 @@ pub fn create_network<'a, 'b: 'a, 'c: 'b>(node_count: usize, cfgs: &'b Vec(node_count: usize, cfgs: &'b Vec>, + secp: Secp256k1, +} + +impl TestWalletSource { + pub fn new(secret_key: SecretKey) -> Self { + Self { + secret_key, + utxos: RefCell::new(Vec::new()), + secp: Secp256k1::new(), + } + } + + pub fn add_utxo(&self, outpoint: bitcoin::OutPoint, value: u64) -> TxOut { + let public_key = bitcoin::PublicKey::new(self.secret_key.public_key(&self.secp)); + let utxo = Utxo::new_p2pkh(outpoint, value, &public_key.pubkey_hash()); + self.utxos.borrow_mut().push(utxo.clone()); + utxo.output + } + + pub fn add_custom_utxo(&self, utxo: Utxo) -> TxOut { + let output = utxo.output.clone(); + self.utxos.borrow_mut().push(utxo); + output + } + + pub fn remove_utxo(&self, outpoint: bitcoin::OutPoint) { + self.utxos.borrow_mut().retain(|utxo| utxo.outpoint != outpoint); + } +} + +impl WalletSource for TestWalletSource { + fn list_confirmed_utxos(&self) -> Result, ()> { + Ok(self.utxos.borrow().clone()) + } + + fn get_change_script(&self) -> Result { + let public_key = bitcoin::PublicKey::new(self.secret_key.public_key(&self.secp)); + Ok(Script::new_p2pkh(&public_key.pubkey_hash())) + } + + fn sign_tx(&self, tx: &mut Transaction) -> Result<(), ()> { + let utxos = self.utxos.borrow(); + for i in 0..tx.input.len() { + if let Some(utxo) = utxos.iter().find(|utxo| utxo.outpoint == tx.input[i].previous_output) { + let sighash = SighashCache::new(&*tx) + .legacy_signature_hash(i, &utxo.output.script_pubkey, EcdsaSighashType::All as u32) + .map_err(|_| ())?; + let sig = self.secp.sign_ecdsa(&sighash.as_hash().into(), &self.secret_key); + let bitcoin_sig = bitcoin::EcdsaSig { sig, hash_ty: EcdsaSighashType::All }.to_vec(); + tx.input[i].script_sig = Builder::new() + .push_slice(&bitcoin_sig) + .push_slice(&self.secret_key.public_key(&self.secp).serialize()) + .into_script(); + } + } + Ok(()) + } +} From ff474ba3841843a973547bacf1ed056ef6045453 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 11 Jul 2023 15:18:08 -0700 Subject: [PATCH 8/8] Integrate BumpTransactionEventHandler into existing anchor tests --- lightning/src/ln/monitor_tests.rs | 202 +++++++++++------------------- 1 file changed, 70 insertions(+), 132 deletions(-) diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index 22b5fa7ce34..a916dbfc9e2 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -9,14 +9,13 @@ //! Further functional tests which test blockchain reorganizations. -use crate::sign::{ChannelSigner, EcdsaChannelSigner}; +use crate::sign::EcdsaChannelSigner; use crate::chain::channelmonitor::{ANTI_REORG_DELAY, LATENCY_GRACE_PERIOD_BLOCKS, Balance}; use crate::chain::transaction::OutPoint; -use crate::chain::chaininterface::LowerBoundedFeeEstimator; -use crate::events::bump_transaction::BumpTransactionEvent; +use crate::chain::chaininterface::{LowerBoundedFeeEstimator, compute_feerate_sat_per_1000_weight}; +use crate::events::bump_transaction::{BumpTransactionEvent, WalletSource}; use crate::events::{Event, MessageSendEvent, MessageSendEventsProvider, ClosureReason, HTLCDestination}; use crate::ln::channel; -use crate::ln::chan_utils; use crate::ln::channelmanager::{BREAKDOWN_TIMEOUT, ChannelManager, PaymentId, RecipientOnionFields}; use crate::ln::msgs::ChannelMessageHandler; use crate::util::config::UserConfig; @@ -1743,6 +1742,17 @@ fn do_test_monitor_rebroadcast_pending_claims(anchors: bool) { check_closed_event(&nodes[0], 1, ClosureReason::CommitmentTxConfirmed, false); check_added_monitors(&nodes[0], 1); + let coinbase_tx = Transaction { + version: 2, + lock_time: PackedLockTime::ZERO, + input: vec![TxIn { ..Default::default() }], + output: vec![TxOut { // UTXO to attach fees to `htlc_tx` on anchors + value: Amount::ONE_BTC.to_sat(), + script_pubkey: nodes[0].wallet_source.get_change_script().unwrap(), + }], + }; + nodes[0].wallet_source.add_utxo(bitcoin::OutPoint { txid: coinbase_tx.txid(), vout: 0 }, coinbase_tx.output[0].value); + // Set up a helper closure we'll use throughout our test. We should only expect retries without // bumps if fees have not increased after a block has been connected (assuming the height timer // re-evaluates at every block) or after `ChainMonitor::rebroadcast_pending_claims` is called. @@ -1750,42 +1760,25 @@ fn do_test_monitor_rebroadcast_pending_claims(anchors: bool) { let mut check_htlc_retry = |should_retry: bool, should_bump: bool| -> Option { let (htlc_tx, htlc_tx_feerate) = if anchors { assert!(nodes[0].tx_broadcaster.txn_broadcast().is_empty()); - let mut events = nodes[0].chain_monitor.chain_monitor.get_and_clear_pending_events(); + let events = nodes[0].chain_monitor.chain_monitor.get_and_clear_pending_events(); assert_eq!(events.len(), if should_retry { 1 } else { 0 }); if !should_retry { return None; } - #[allow(unused_assignments)] - let mut tx = Transaction { - version: 2, - lock_time: bitcoin::PackedLockTime::ZERO, - input: vec![], - output: vec![], - }; - #[allow(unused_assignments)] - let mut feerate = 0; - feerate = if let Event::BumpTransaction(BumpTransactionEvent::HTLCResolution { - target_feerate_sat_per_1000_weight, mut htlc_descriptors, tx_lock_time, .. - }) = events.pop().unwrap() { - let secp = Secp256k1::new(); - assert_eq!(htlc_descriptors.len(), 1); - let descriptor = htlc_descriptors.pop().unwrap(); - assert_eq!(descriptor.commitment_txid, commitment_txn[0].txid()); - let htlc_output_idx = descriptor.htlc.transaction_output_index.unwrap() as usize; - assert!(htlc_output_idx < commitment_txn[0].output.len()); - tx.lock_time = tx_lock_time; - // Note that we don't care about actually making the HTLC transaction meet the - // feerate for the test, we just want to make sure the feerates we receive from - // the events never decrease. - tx.input.push(descriptor.unsigned_tx_input()); - tx.output.push(descriptor.tx_output(&secp)); - let signer = descriptor.derive_channel_signer(&nodes[0].keys_manager); - let our_sig = signer.sign_holder_htlc_transaction(&mut tx, 0, &descriptor, &secp).unwrap(); - let witness_script = descriptor.witness_script(&secp); - tx.input[0].witness = descriptor.tx_input_witness(&our_sig, &witness_script); - target_feerate_sat_per_1000_weight as u64 - } else { panic!("unexpected event"); }; - (tx, feerate) + match &events[0] { + Event::BumpTransaction(event) => { + nodes[0].bump_tx_handler.handle_event(&event); + let mut txn = nodes[0].tx_broadcaster.unique_txn_broadcast(); + assert_eq!(txn.len(), 1); + let htlc_tx = txn.pop().unwrap(); + check_spends!(&htlc_tx, &commitment_txn[0], &coinbase_tx); + let htlc_tx_fee = HTLC_AMT_SAT + coinbase_tx.output[0].value - + htlc_tx.output.iter().map(|output| output.value).sum::(); + let htlc_tx_weight = htlc_tx.weight() as u64; + (htlc_tx, compute_feerate_sat_per_1000_weight(htlc_tx_fee, htlc_tx_weight)) + } + _ => panic!("Unexpected event"), + } } else { assert!(nodes[0].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty()); let mut txn = nodes[0].tx_broadcaster.txn_broadcast(); @@ -1796,8 +1789,8 @@ fn do_test_monitor_rebroadcast_pending_claims(anchors: bool) { let htlc_tx = txn.pop().unwrap(); check_spends!(htlc_tx, commitment_txn[0]); let htlc_tx_fee = HTLC_AMT_SAT - htlc_tx.output[0].value; - let htlc_tx_feerate = htlc_tx_fee * 1000 / htlc_tx.weight() as u64; - (htlc_tx, htlc_tx_feerate) + let htlc_tx_weight = htlc_tx.weight() as u64; + (htlc_tx, compute_feerate_sat_per_1000_weight(htlc_tx_fee, htlc_tx_weight)) }; if should_bump { assert!(htlc_tx_feerate > prev_htlc_tx_feerate.take().unwrap()); @@ -1837,9 +1830,11 @@ fn do_test_monitor_rebroadcast_pending_claims(anchors: bool) { // Mine the HTLC transaction to ensure we don't retry claims while they're confirmed. mine_transaction(&nodes[0], &htlc_tx); - // If we have a `ConnectStyle` that advertises the new block first without the transasctions, + // If we have a `ConnectStyle` that advertises the new block first without the transactions, // we'll receive an extra bumped claim. if nodes[0].connect_style.borrow().updates_best_block_first() { + nodes[0].wallet_source.add_utxo(bitcoin::OutPoint { txid: coinbase_tx.txid(), vout: 0 }, coinbase_tx.output[0].value); + nodes[0].wallet_source.remove_utxo(bitcoin::OutPoint { txid: htlc_tx.txid(), vout: 1 }); check_htlc_retry(true, anchors); } nodes[0].chain_monitor.chain_monitor.rebroadcast_pending_claims(); @@ -1860,7 +1855,6 @@ fn test_yield_anchors_events() { // allowing the consumer to provide additional fees to the commitment transaction to be // broadcast. Once the commitment transaction confirms, events for the HTLC resolution should be // emitted by LDK, such that the consumer can attach fees to the zero fee HTLC transactions. - let secp = Secp256k1::new(); let mut chanmon_cfgs = create_chanmon_cfgs(2); let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); let mut anchors_config = UserConfig::default(); @@ -1891,26 +1885,23 @@ fn test_yield_anchors_events() { let mut holder_events = nodes[0].chain_monitor.chain_monitor.get_and_clear_pending_events(); assert_eq!(holder_events.len(), 1); let (commitment_tx, anchor_tx) = match holder_events.pop().unwrap() { - Event::BumpTransaction(BumpTransactionEvent::ChannelClose { commitment_tx, anchor_descriptor, .. }) => { - assert_eq!(commitment_tx.input.len(), 1); - assert_eq!(commitment_tx.output.len(), 6); - let mut anchor_tx = Transaction { + Event::BumpTransaction(event) => { + let coinbase_tx = Transaction { version: 2, lock_time: PackedLockTime::ZERO, - input: vec![ - TxIn { previous_output: anchor_descriptor.outpoint, ..Default::default() }, - TxIn { ..Default::default() }, - ], - output: vec![TxOut { + input: vec![TxIn { ..Default::default() }], + output: vec![TxOut { // UTXO to attach fees to `anchor_tx` value: Amount::ONE_BTC.to_sat(), - script_pubkey: Script::new_op_return(&[]), + script_pubkey: nodes[0].wallet_source.get_change_script().unwrap(), }], }; - let signer = anchor_descriptor.derive_channel_signer(&nodes[0].keys_manager); - let funding_sig = signer.sign_holder_anchor_input(&mut anchor_tx, 0, &secp).unwrap(); - anchor_tx.input[0].witness = chan_utils::build_anchor_input_witness( - &signer.pubkeys().funding_pubkey, &funding_sig - ); + nodes[0].wallet_source.add_utxo(bitcoin::OutPoint { txid: coinbase_tx.txid(), vout: 0 }, coinbase_tx.output[0].value); + nodes[0].bump_tx_handler.handle_event(&event); + let mut txn = nodes[0].tx_broadcaster.unique_txn_broadcast(); + assert_eq!(txn.len(), 2); + let anchor_tx = txn.pop().unwrap(); + let commitment_tx = txn.pop().unwrap(); + check_spends!(anchor_tx, coinbase_tx, commitment_tx); (commitment_tx, anchor_tx) }, _ => panic!("Unexpected event"), @@ -1934,28 +1925,12 @@ fn test_yield_anchors_events() { let mut htlc_txs = Vec::with_capacity(2); for event in holder_events { match event { - Event::BumpTransaction(BumpTransactionEvent::HTLCResolution { htlc_descriptors, tx_lock_time, .. }) => { - assert_eq!(htlc_descriptors.len(), 1); - let htlc_descriptor = &htlc_descriptors[0]; - let mut htlc_tx = Transaction { - version: 2, - lock_time: tx_lock_time, - input: vec![ - htlc_descriptor.unsigned_tx_input(), // HTLC input - TxIn { ..Default::default() } // Fee input - ], - output: vec![ - htlc_descriptor.tx_output(&secp), // HTLC output - TxOut { // Fee input change - value: Amount::ONE_BTC.to_sat(), - script_pubkey: Script::new_op_return(&[]), - } - ] - }; - let signer = htlc_descriptor.derive_channel_signer(&nodes[0].keys_manager); - let our_sig = signer.sign_holder_htlc_transaction(&mut htlc_tx, 0, htlc_descriptor, &secp).unwrap(); - let witness_script = htlc_descriptor.witness_script(&secp); - htlc_tx.input[0].witness = htlc_descriptor.tx_input_witness(&our_sig, &witness_script); + Event::BumpTransaction(event) => { + nodes[0].bump_tx_handler.handle_event(&event); + let mut txn = nodes[0].tx_broadcaster.unique_txn_broadcast(); + assert_eq!(txn.len(), 1); + let htlc_tx = txn.pop().unwrap(); + check_spends!(htlc_tx, commitment_tx, anchor_tx); htlc_txs.push(htlc_tx); }, _ => panic!("Unexpected event"), @@ -2060,7 +2035,7 @@ fn test_anchors_aggregated_revoked_htlc_tx() { check_added_monitors(&nodes[1], 2); check_closed_event!(&nodes[1], 2, ClosureReason::OutdatedChannelManager); let (revoked_commitment_a, revoked_commitment_b) = { - let txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); + let txn = nodes[1].tx_broadcaster.unique_txn_broadcast(); assert_eq!(txn.len(), 2); assert_eq!(txn[0].output.len(), 6); // 2 HTLC outputs + 1 to_self output + 1 to_remote output + 2 anchor outputs assert_eq!(txn[1].output.len(), 6); // 2 HTLC outputs + 1 to_self output + 1 to_remote output + 2 anchor outputs @@ -2079,71 +2054,32 @@ fn test_anchors_aggregated_revoked_htlc_tx() { assert!(nodes[0].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty()); let events = nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events(); assert_eq!(events.len(), 2); - let anchor_tx = { - let secret_key = SecretKey::from_slice(&[1; 32]).unwrap(); - let public_key = PublicKey::new(secret_key.public_key(&secp)); - let fee_utxo_script = Script::new_v0_p2wpkh(&public_key.wpubkey_hash().unwrap()); + let mut anchor_txs = Vec::with_capacity(events.len()); + for (idx, event) in events.into_iter().enumerate() { + let utxo_value = Amount::ONE_BTC.to_sat() * (idx + 1) as u64; let coinbase_tx = Transaction { version: 2, lock_time: PackedLockTime::ZERO, input: vec![TxIn { ..Default::default() }], output: vec![TxOut { // UTXO to attach fees to `anchor_tx` - value: Amount::ONE_BTC.to_sat(), - script_pubkey: fee_utxo_script.clone(), + value: utxo_value, + script_pubkey: nodes[1].wallet_source.get_change_script().unwrap(), }], }; - let mut anchor_tx = Transaction { - version: 2, - lock_time: PackedLockTime::ZERO, - input: vec![ - TxIn { // Fee input - previous_output: bitcoin::OutPoint { txid: coinbase_tx.txid(), vout: 0 }, - ..Default::default() - }, - ], - output: vec![TxOut { // Fee input change - value: coinbase_tx.output[0].value / 2 , - script_pubkey: Script::new_op_return(&[]), - }], - }; - let mut signers = Vec::with_capacity(2); - for event in events { - match event { - Event::BumpTransaction(BumpTransactionEvent::ChannelClose { anchor_descriptor, .. }) => { - anchor_tx.input.push(TxIn { - previous_output: anchor_descriptor.outpoint, - ..Default::default() - }); - let signer = anchor_descriptor.derive_channel_signer(&nodes[1].keys_manager); - signers.push(signer); - }, - _ => panic!("Unexpected event"), - } - } - for (i, signer) in signers.into_iter().enumerate() { - let anchor_idx = i + 1; - let funding_sig = signer.sign_holder_anchor_input(&mut anchor_tx, anchor_idx, &secp).unwrap(); - anchor_tx.input[anchor_idx].witness = chan_utils::build_anchor_input_witness( - &signer.pubkeys().funding_pubkey, &funding_sig - ); - } - let fee_utxo_sig = { - let witness_script = Script::new_p2pkh(&public_key.pubkey_hash()); - let sighash = hash_to_message!(&SighashCache::new(&anchor_tx).segwit_signature_hash( - 0, &witness_script, coinbase_tx.output[0].value, EcdsaSighashType::All - ).unwrap()[..]); - let sig = sign(&secp, &sighash, &secret_key); - let mut sig = sig.serialize_der().to_vec(); - sig.push(EcdsaSighashType::All as u8); - sig + nodes[1].wallet_source.add_utxo(bitcoin::OutPoint { txid: coinbase_tx.txid(), vout: 0 }, utxo_value); + match event { + Event::BumpTransaction(event) => nodes[1].bump_tx_handler.handle_event(&event), + _ => panic!("Unexpected event"), }; - anchor_tx.input[0].witness = Witness::from_vec(vec![fee_utxo_sig, public_key.to_bytes()]); - check_spends!(anchor_tx, coinbase_tx, revoked_commitment_a, revoked_commitment_b); - anchor_tx + let txn = nodes[1].tx_broadcaster.txn_broadcast(); + assert_eq!(txn.len(), 2); + let (commitment_tx, anchor_tx) = (&txn[0], &txn[1]); + check_spends!(anchor_tx, coinbase_tx, commitment_tx); + anchor_txs.push(anchor_tx.clone()); }; for node in &nodes { - mine_transactions(node, &[&revoked_commitment_a, &revoked_commitment_b, &anchor_tx]); + mine_transactions(node, &[&revoked_commitment_a, &anchor_txs[0], &revoked_commitment_b, &anchor_txs[1]]); } check_added_monitors!(&nodes[0], 2); check_closed_broadcast(&nodes[0], 2, true); @@ -2213,6 +2149,8 @@ fn test_anchors_aggregated_revoked_htlc_tx() { }; let mut descriptors = Vec::with_capacity(4); for event in events { + // We don't use the `BumpTransactionEventHandler` here because it does not support + // creating one transaction from multiple `HTLCResolution` events. if let Event::BumpTransaction(BumpTransactionEvent::HTLCResolution { mut htlc_descriptors, tx_lock_time, .. }) = event { assert_eq!(htlc_descriptors.len(), 2); for htlc_descriptor in &htlc_descriptors {