From 2ac09711d3e2c329d96e0bd60938ba83ff382b37 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 28 Mar 2023 12:03:19 -0700 Subject: [PATCH 1/3] Re-work PackageSolvingData::absolute_tx_timelock Previously, this would return the earliest height the output could be confirmed, which seems to no longer be useful. The only use of the method was to determine whether we should delay a package to a future block. Instead, we choose to return the absolute locktime the transaction spending the output should have, which better corresponds to the method name and still supports the delay functionality mentioned. Doing so also allows us to expose the locktime required for HTLC transactions we need to broadcast based on our own commitments for anchor channels. --- lightning/src/chain/onchaintx.rs | 9 ++--- lightning/src/chain/package.rs | 60 ++++++++++++++++++++++++-------- 2 files changed, 51 insertions(+), 18 deletions(-) diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 2c570f580bd..0ff0df59cf2 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -654,16 +654,17 @@ impl OnchainTxHandler .find(|locked_package| locked_package.outpoints() == req.outpoints()); if let Some(package) = timelocked_equivalent_package { log_info!(logger, "Ignoring second claim for outpoint {}:{}, we already have one which we're waiting on a timelock at {} for.", - req.outpoints()[0].txid, req.outpoints()[0].vout, package.package_timelock()); + req.outpoints()[0].txid, req.outpoints()[0].vout, package.package_locktime(cur_height)); continue; } - if req.package_timelock() > cur_height + 1 { - log_info!(logger, "Delaying claim of package until its timelock at {} (current height {}), the following outpoints are spent:", req.package_timelock(), cur_height); + let package_locktime = req.package_locktime(cur_height); + if package_locktime > cur_height + 1 { + log_info!(logger, "Delaying claim of package until its timelock at {} (current height {}), the following outpoints are spent:", package_locktime, cur_height); for outpoint in req.outpoints() { log_info!(logger, " Outpoint {}", outpoint); } - self.locktimed_packages.entry(req.package_timelock()).or_insert(Vec::new()).push(req); + self.locktimed_packages.entry(package_locktime).or_insert(Vec::new()).push(req); continue; } diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index 5537a56f2f3..2ab37484148 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -460,18 +460,23 @@ impl PackageSolvingData { _ => { panic!("API Error!"); } } } - fn absolute_tx_timelock(&self, output_conf_height: u32) -> u32 { - // Get the absolute timelock at which this output can be spent given the height at which - // this output was confirmed. We use `output_conf_height + 1` as a safe default as we can - // be confirmed in the next block and transactions with time lock `current_height + 1` - // always propagate. + fn absolute_tx_timelock(&self, current_height: u32) -> u32 { + // We use `current_height + 1` as our default locktime to discourage fee sniping and because + // transactions with it always propagate. let absolute_timelock = match self { - PackageSolvingData::RevokedOutput(_) => output_conf_height + 1, - PackageSolvingData::RevokedHTLCOutput(_) => output_conf_height + 1, - PackageSolvingData::CounterpartyOfferedHTLCOutput(_) => output_conf_height + 1, - PackageSolvingData::CounterpartyReceivedHTLCOutput(ref outp) => cmp::max(outp.htlc.cltv_expiry, output_conf_height + 1), - PackageSolvingData::HolderHTLCOutput(ref outp) => cmp::max(outp.cltv_expiry, output_conf_height + 1), - PackageSolvingData::HolderFundingOutput(_) => output_conf_height + 1, + PackageSolvingData::RevokedOutput(_) => current_height + 1, + PackageSolvingData::RevokedHTLCOutput(_) => current_height + 1, + PackageSolvingData::CounterpartyOfferedHTLCOutput(_) => current_height + 1, + PackageSolvingData::CounterpartyReceivedHTLCOutput(ref outp) => cmp::max(outp.htlc.cltv_expiry, current_height + 1), + // HTLC timeout/success transactions rely on a fixed timelock due to the counterparty's + // signature. + PackageSolvingData::HolderHTLCOutput(ref outp) => { + if outp.preimage.is_some() { + debug_assert_eq!(outp.cltv_expiry, 0); + } + outp.cltv_expiry + }, + PackageSolvingData::HolderFundingOutput(_) => current_height + 1, }; absolute_timelock } @@ -638,9 +643,36 @@ impl PackageTemplate { } amounts } - pub(crate) fn package_timelock(&self) -> u32 { - self.inputs.iter().map(|(_, outp)| outp.absolute_tx_timelock(self.height_original)) - .max().expect("There must always be at least one output to spend in a PackageTemplate") + pub(crate) fn package_locktime(&self, current_height: u32) -> u32 { + let locktime = self.inputs.iter().map(|(_, outp)| outp.absolute_tx_timelock(current_height)) + .max().expect("There must always be at least one output to spend in a PackageTemplate"); + + // If we ever try to aggregate a `HolderHTLCOutput`s with another output type, we'll likely + // end up with an incorrect transaction locktime since the counterparty has included it in + // its HTLC signature. This should never happen unless we decide to aggregate outputs across + // different channel commitments. + #[cfg(debug_assertions)] { + if self.inputs.iter().any(|(_, outp)| + if let PackageSolvingData::HolderHTLCOutput(outp) = outp { + outp.preimage.is_some() + } else { + false + } + ) { + debug_assert_eq!(locktime, 0); + }; + for timeout_htlc_expiry in self.inputs.iter().filter_map(|(_, outp)| + if let PackageSolvingData::HolderHTLCOutput(outp) = outp { + if outp.preimage.is_none() { + Some(outp.cltv_expiry) + } else { None } + } else { None } + ) { + debug_assert_eq!(locktime, timeout_htlc_expiry); + } + } + + locktime } pub(crate) fn package_weight(&self, destination_script: &Script) -> usize { let mut inputs_weight = 0; From 68122bd09de5feca5538247f2533ec3b89ccf242 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Tue, 28 Mar 2023 12:09:13 -0700 Subject: [PATCH 2/3] Set transaction locktime on malleable packages to discourage fee sniping This only applies to all malleable packages on channels pre-dating anchors and malleables packages for counterparty commitments post-anchors. Malleables packages for holder commitments post-anchors should have their transaction locktime applied manually by the consumer of `BumpTransactionEvent::HTLCResolution` events. --- lightning/src/chain/onchaintx.rs | 4 +++- lightning/src/chain/package.rs | 6 +++--- lightning/src/ln/functional_tests.rs | 6 +++--- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 0ff0df59cf2..c95aec9774a 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -558,7 +558,9 @@ impl OnchainTxHandler ) { assert!(new_feerate != 0); - let transaction = cached_request.finalize_malleable_package(self, output_value, self.destination_script.clone(), logger).unwrap(); + let transaction = cached_request.finalize_malleable_package( + cur_height, self, output_value, self.destination_script.clone(), logger + ).unwrap(); log_trace!(logger, "...with timer {} and feerate {}", new_timer.unwrap(), new_feerate); assert!(predicted_weight >= transaction.weight()); return Some((new_timer, new_feerate, OnchainClaim::Tx(transaction))); diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index 2ab37484148..e8886e5a2aa 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -434,7 +434,6 @@ impl PackageSolvingData { let chan_keys = TxCreationKeys::derive_new(&onchain_handler.secp_ctx, &outp.per_commitment_point, &outp.counterparty_delayed_payment_base_key, &outp.counterparty_htlc_base_key, &onchain_handler.signer.pubkeys().revocation_basepoint, &onchain_handler.signer.pubkeys().htlc_basepoint); let witness_script = chan_utils::get_htlc_redeemscript_with_explicit_keys(&outp.htlc, onchain_handler.opt_anchors(), &chan_keys.broadcaster_htlc_key, &chan_keys.countersignatory_htlc_key, &chan_keys.revocation_key); - bumped_tx.lock_time = PackedLockTime(outp.htlc.cltv_expiry); // Right now we don't aggregate time-locked transaction, if we do we should set lock_time before to avoid breaking hash computation if let Ok(sig) = onchain_handler.signer.sign_counterparty_htlc_transaction(&bumped_tx, i, &outp.htlc.amount_msat / 1000, &outp.per_commitment_point, &outp.htlc, &onchain_handler.secp_ctx) { let mut ser_sig = sig.serialize_der().to_vec(); ser_sig.push(EcdsaSighashType::All as u8); @@ -708,12 +707,13 @@ impl PackageTemplate { htlcs } pub(crate) fn finalize_malleable_package( - &self, onchain_handler: &mut OnchainTxHandler, value: u64, destination_script: Script, logger: &L + &self, current_height: u32, onchain_handler: &mut OnchainTxHandler, value: u64, + destination_script: Script, logger: &L ) -> Option where L::Target: Logger { debug_assert!(self.is_malleable()); let mut bumped_tx = Transaction { version: 2, - lock_time: PackedLockTime::ZERO, + lock_time: PackedLockTime(self.package_locktime(current_height)), input: vec![], output: vec![TxOut { script_pubkey: destination_script, diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 265bd49ac92..38a78ab6563 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -2843,7 +2843,7 @@ fn test_htlc_on_chain_success() { assert_eq!(commitment_spend.input.len(), 2); assert_eq!(commitment_spend.input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT); assert_eq!(commitment_spend.input[1].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT); - assert_eq!(commitment_spend.lock_time.0, 0); + assert_eq!(commitment_spend.lock_time.0, nodes[1].best_block_info().1 + 1); assert!(commitment_spend.output[0].script_pubkey.is_v0_p2wpkh()); // direct payment // We don't bother to check that B can claim the HTLC output on its commitment tx here as // we already checked the same situation with A. @@ -4699,7 +4699,7 @@ fn test_onchain_to_onchain_claim() { check_spends!(b_txn[0], commitment_tx[0]); assert_eq!(b_txn[0].input[0].witness.clone().last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT); assert!(b_txn[0].output[0].script_pubkey.is_v0_p2wpkh()); // direct payment - assert_eq!(b_txn[0].lock_time.0, 0); // Success tx + assert_eq!(b_txn[0].lock_time.0, nodes[1].best_block_info().1 + 1); // Success tx check_closed_broadcast!(nodes[1], true); check_added_monitors!(nodes[1], 1); @@ -6860,7 +6860,7 @@ fn do_test_sweep_outbound_htlc_failure_update(revoked: bool, local: bool) { if !revoked { assert_eq!(timeout_tx[0].input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT); } else { - assert_eq!(timeout_tx[0].lock_time.0, 0); + assert_eq!(timeout_tx[0].lock_time.0, 12); } // We fail non-dust-HTLC 2 by broadcast of local timeout/revocation-claim tx mine_transaction(&nodes[0], &timeout_tx[0]); From 23e233ba257c27b14d0d6167f1d87d9c1bf5e344 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Mon, 6 Mar 2023 17:01:21 -0800 Subject: [PATCH 3/3] Expose HTLC transaction locktime in BumpTransactionEvent::HTLCResolution While users could easily figure it out based on the set of HTLC descriptors included within, we already track it within the `OnchainTxHandler`, so we might as well expose it to users as a nice-to-have. It's also yet another thing they must get right to ensure their HTLC transaction broadcasts are valid. --- lightning/src/chain/channelmonitor.rs | 3 ++- lightning/src/chain/onchaintx.rs | 4 ++++ lightning/src/events/bump_transaction.rs | 2 ++ lightning/src/ln/monitor_tests.rs | 11 ++++------- 4 files changed, 12 insertions(+), 8 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 7a69be7dcdf..9044eec63e4 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -2426,7 +2426,7 @@ impl ChannelMonitorImpl { })); }, ClaimEvent::BumpHTLC { - target_feerate_sat_per_1000_weight, htlcs, + target_feerate_sat_per_1000_weight, htlcs, tx_lock_time, } => { let mut htlc_descriptors = Vec::with_capacity(htlcs.len()); for htlc in htlcs { @@ -2444,6 +2444,7 @@ impl ChannelMonitorImpl { ret.push(Event::BumpTransaction(BumpTransactionEvent::HTLCResolution { target_feerate_sat_per_1000_weight, htlc_descriptors, + tx_lock_time, })); } } diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index c95aec9774a..f690d366427 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -12,6 +12,8 @@ //! OnchainTxHandler objects are fully-part of ChannelMonitor and encapsulates all //! building, tracking, bumping and notifications functions. +#[cfg(anchors)] +use bitcoin::PackedLockTime; use bitcoin::blockdata::transaction::Transaction; use bitcoin::blockdata::transaction::OutPoint as BitcoinOutPoint; use bitcoin::blockdata::script::Script; @@ -201,6 +203,7 @@ pub(crate) enum ClaimEvent { BumpHTLC { target_feerate_sat_per_1000_weight: u32, htlcs: Vec, + tx_lock_time: PackedLockTime, }, } @@ -544,6 +547,7 @@ impl OnchainTxHandler OnchainClaim::Event(ClaimEvent::BumpHTLC { target_feerate_sat_per_1000_weight, htlcs, + tx_lock_time: PackedLockTime(cached_request.package_locktime(cur_height)), }), )); } else { diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs index f56a99df8f2..6a3360a4d7e 100644 --- a/lightning/src/events/bump_transaction.rs +++ b/lightning/src/events/bump_transaction.rs @@ -227,5 +227,7 @@ pub enum BumpTransactionEvent { /// The set of pending HTLCs on the confirmed commitment that need to be claimed, preferably /// by the same transaction. htlc_descriptors: Vec, + /// The locktime required for the resulting HTLC transaction. + tx_lock_time: PackedLockTime, }, } diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index f4dedd89eb9..15d0167ad46 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -1775,7 +1775,7 @@ 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, .. }) => { + Event::BumpTransaction(BumpTransactionEvent::HTLCResolution { htlc_descriptors, tx_lock_time, .. }) => { assert_eq!(htlc_descriptors.len(), 1); let htlc_descriptor = &htlc_descriptors[0]; let signer = nodes[0].keys_manager.derive_channel_keys( @@ -1784,11 +1784,7 @@ fn test_yield_anchors_events() { let per_commitment_point = signer.get_per_commitment_point(htlc_descriptor.per_commitment_number, &secp); let mut htlc_tx = Transaction { version: 2, - lock_time: if htlc_descriptor.htlc.offered { - PackedLockTime(htlc_descriptor.htlc.cltv_expiry) - } else { - PackedLockTime::ZERO - }, + lock_time: tx_lock_time, input: vec![ htlc_descriptor.unsigned_tx_input(), // HTLC input TxIn { ..Default::default() } // Fee input @@ -2064,7 +2060,7 @@ fn test_anchors_aggregated_revoked_htlc_tx() { }; let mut descriptors = Vec::with_capacity(4); for event in events { - if let Event::BumpTransaction(BumpTransactionEvent::HTLCResolution { mut htlc_descriptors, .. }) = event { + 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 { assert!(!htlc_descriptor.htlc.offered); @@ -2076,6 +2072,7 @@ fn test_anchors_aggregated_revoked_htlc_tx() { htlc_tx.output.push(htlc_descriptor.tx_output(&per_commitment_point, &secp)); } descriptors.append(&mut htlc_descriptors); + htlc_tx.lock_time = tx_lock_time; } else { panic!("Unexpected event"); }