Merge pull request #2082 from wpaulino/bump-htlc-resolution-tx-locktime

Expose HTLC transaction locktime in BumpTransactionEvent::HTLCResolution

Author: arik-so

lightning/src/chain/channelmonitor.rs

@@ -2426,7 +2426,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 					}));
 				},
 				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<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 					ret.push(Event::BumpTransaction(BumpTransactionEvent::HTLCResolution {
 						target_feerate_sat_per_1000_weight,
 						htlc_descriptors,
+						tx_lock_time,
 					}));
 				}
 			}

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<ExternalHTLCClaim>,
+		tx_lock_time: PackedLockTime,
 	},
 }
 
@@ -544,6 +547,7 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
 							OnchainClaim::Event(ClaimEvent::BumpHTLC {
 								target_feerate_sat_per_1000_weight,
 								htlcs,
+								tx_lock_time: PackedLockTime(cached_request.package_locktime(cur_height)),
 							}),
 						));
 					} else {
@@ -558,7 +562,9 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
 			) {
 				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)));
@@ -654,16 +660,17 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
 					.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;
 				}
 

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);
@@ -460,18 +459,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 +642,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;
@@ -676,12 +707,13 @@ impl PackageTemplate {
 		htlcs
 	}
 	pub(crate) fn finalize_malleable_package<L: Deref, Signer: WriteableEcdsaChannelSigner>(
-		&self, onchain_handler: &mut OnchainTxHandler<Signer>, value: u64, destination_script: Script, logger: &L
+		&self, current_height: u32, onchain_handler: &mut OnchainTxHandler<Signer>, value: u64,
+		destination_script: Script, logger: &L
 	) -> Option<Transaction> 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,

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<HTLCDescriptor>,
+		/// The locktime required for the resulting HTLC transaction.
+		tx_lock_time: PackedLockTime,
 	},
 }

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]);

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");
 			}