Merge pull request #3564 from TheBlueMatt/2025-01-revoked-htlc-not-pinnable

Set correct `counterparty_spendable_height` on c.p. revoked HTLCs

Author: TheBlueMatt

lightning/src/chain/channelmonitor.rs

@@ -3564,11 +3564,16 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 							return (claimable_outpoints, to_counterparty_output_info);
 						}
 						let revk_htlc_outp = RevokedHTLCOutput::build(per_commitment_point, self.counterparty_commitment_params.counterparty_delayed_payment_base_key, self.counterparty_commitment_params.counterparty_htlc_base_key, per_commitment_key, htlc.amount_msat / 1000, htlc.clone(), &self.onchain_tx_handler.channel_transaction_parameters.channel_type_features);
+						let counterparty_spendable_height = if htlc.offered {
+							htlc.cltv_expiry
+						} else {
+							height
+						};
 						let justice_package = PackageTemplate::build_package(
 							commitment_txid,
 							transaction_output_index,
 							PackageSolvingData::RevokedHTLCOutput(revk_htlc_outp),
-							htlc.cltv_expiry,
+							counterparty_spendable_height,
 						);
 						claimable_outpoints.push(justice_package);
 					}

lightning/src/chain/package.rs

@@ -699,8 +699,13 @@ impl PackageSolvingData {
 		match self {
 			PackageSolvingData::RevokedOutput(RevokedOutput { .. }) =>
 				PackageMalleability::Malleable(AggregationCluster::Unpinnable),
-			PackageSolvingData::RevokedHTLCOutput(..) =>
-				PackageMalleability::Malleable(AggregationCluster::Pinnable),
+			PackageSolvingData::RevokedHTLCOutput(RevokedHTLCOutput { htlc, .. }) => {
+				if htlc.offered {
+					PackageMalleability::Malleable(AggregationCluster::Unpinnable)
+				} else {
+					PackageMalleability::Malleable(AggregationCluster::Pinnable)
+				}
+			},
 			PackageSolvingData::CounterpartyOfferedHTLCOutput(..) =>
 				PackageMalleability::Malleable(AggregationCluster::Unpinnable),
 			PackageSolvingData::CounterpartyReceivedHTLCOutput(..) =>
@@ -771,10 +776,12 @@ pub struct PackageTemplate {
 	/// Block height at which our counterparty can potentially claim this output as well (assuming
 	/// they have the keys or information required to do so).
 	///
-	/// This is used primarily by external consumers to decide when an output becomes "pinnable"
-	/// because the counterparty can potentially spend it. It is also used internally by
-	/// [`Self::get_height_timer`] to identify when an output must be claimed by, depending on the
-	/// type of output.
+	/// This is used primarily to decide when an output becomes "pinnable" because the counterparty
+	/// can potentially spend it. It is also used internally by [`Self::get_height_timer`] to
+	/// identify when an output must be claimed by, depending on the type of output.
+	///
+	/// Note that for revoked counterparty HTLC outputs the value may be zero in some cases where
+	/// we upgraded from LDK 0.1 or prior.
 	counterparty_spendable_height: u32,
 	// Cache of package feerate committed at previous (re)broadcast. If bumping resources
 	// (either claimed output value or external utxo), it will keep increasing until holder
@@ -834,17 +841,17 @@ impl PackageTemplate {
 				// Now check that we only merge packages if they are both unpinnable or both
 				// pinnable.
 				let self_pinnable = self_cluster == AggregationCluster::Pinnable ||
-					self.counterparty_spendable_height() <= cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
+					self.counterparty_spendable_height <= cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
 				let other_pinnable = other_cluster == AggregationCluster::Pinnable ||
-					other.counterparty_spendable_height() <= cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
+					other.counterparty_spendable_height <= cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
 				if self_pinnable && other_pinnable {
 					return true;
 				}
 
 				let self_unpinnable = self_cluster == AggregationCluster::Unpinnable &&
-					self.counterparty_spendable_height() > cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
+					self.counterparty_spendable_height > cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
 				let other_unpinnable = other_cluster == AggregationCluster::Unpinnable &&
-					other.counterparty_spendable_height() > cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
+					other.counterparty_spendable_height > cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
 				if self_unpinnable && other_unpinnable {
 					return true;
 				}
@@ -855,13 +862,6 @@ impl PackageTemplate {
 	pub(crate) fn is_malleable(&self) -> bool {
 		matches!(self.malleability, PackageMalleability::Malleable(..))
 	}
-	/// The height at which our counterparty may be able to spend this output.
-	///
-	/// This is an important limit for aggregation as after this height our counterparty may be
-	/// able to pin transactions spending this output in the mempool.
-	pub(crate) fn counterparty_spendable_height(&self) -> u32 {
-		self.counterparty_spendable_height
-	}
 	pub(crate) fn previous_feerate(&self) -> u64 {
 		self.feerate_previous
 	}
@@ -1225,6 +1225,18 @@ impl Readable for PackageTemplate {
 			(4, _height_original, option), // Written with a dummy value since 0.1
 			(6, height_timer, option),
 		});
+		for (_, input) in &inputs {
+			if let PackageSolvingData::RevokedHTLCOutput(RevokedHTLCOutput { htlc, .. }) = input {
+				// LDK versions through 0.1 set the wrong counterparty_spendable_height for
+				// non-offered revoked HTLCs (ie HTLCs we sent to our counterparty which they can
+				// claim with a preimage immediately). Here we detect this and reset the value to
+				// zero, as the value is unused except for merging decisions which doesn't care
+				// about any values below the current height.
+				if !htlc.offered && htlc.cltv_expiry == counterparty_spendable_height {
+					counterparty_spendable_height = 0;
+				}
+			}
+		}
 		Ok(PackageTemplate {
 			inputs,
 			malleability,

lightning/src/ln/functional_tests.rs

@@ -15,7 +15,7 @@ use crate::chain;
 use crate::chain::{ChannelMonitorUpdateStatus, Confirm, Listen, Watch};
 use crate::chain::chaininterface::LowerBoundedFeeEstimator;
 use crate::chain::channelmonitor;
-use crate::chain::channelmonitor::{Balance, ChannelMonitorUpdateStep, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY};
+use crate::chain::channelmonitor::{Balance, ChannelMonitorUpdateStep, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY, COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE};
 use crate::chain::transaction::OutPoint;
 use crate::sign::{ecdsa::EcdsaChannelSigner, EntropySource, OutputSpender, SignerProvider};
 use crate::events::bump_transaction::WalletSource;
@@ -2631,14 +2631,12 @@ fn test_justice_tx_htlc_timeout() {
 		mine_transaction(&nodes[1], &revoked_local_txn[0]);
 		{
 			let mut node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
-			// The unpinnable, revoked to_self output, and the pinnable, revoked htlc output will
-			// be claimed in separate transactions.
-			assert_eq!(node_txn.len(), 2);
-			for tx in node_txn.iter() {
-				assert_eq!(tx.input.len(), 1);
-				check_spends!(tx, revoked_local_txn[0]);
-			}
-			assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output);
+			// The revoked HTLC output is not pinnable for another `TEST_FINAL_CLTV` blocks, and is
+			// thus claimed in the same transaction with the revoked to_self output.
+			assert_eq!(node_txn.len(), 1);
+			assert_eq!(node_txn[0].input.len(), 2);
+			check_spends!(node_txn[0], revoked_local_txn[0]);
+			assert_ne!(node_txn[0].input[0].previous_output, node_txn[0].input[1].previous_output);
 			node_txn.clear();
 		}
 		check_added_monitors!(nodes[1], 1);
@@ -2858,28 +2856,26 @@ fn claim_htlc_outputs() {
 		assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
 
 		let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
-		assert_eq!(node_txn.len(), 2); // Two penalty transactions:
-		assert_eq!(node_txn[0].input.len(), 1); // Claims the unpinnable, revoked output.
-		assert_eq!(node_txn[1].input.len(), 2); // Claims both pinnable, revoked HTLC outputs separately.
-		check_spends!(node_txn[0], revoked_local_txn[0]);
-		check_spends!(node_txn[1], revoked_local_txn[0]);
-		assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output);
-		assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[1].previous_output);
-		assert_ne!(node_txn[1].input[0].previous_output, node_txn[1].input[1].previous_output);
+		assert_eq!(node_txn.len(), 2); // ChannelMonitor: penalty txn
+
+		// The ChannelMonitor should claim the accepted HTLC output separately from the offered
+		// HTLC and to_self outputs.
+		let accepted_claim = node_txn.iter().filter(|tx| tx.input.len() == 1).next().unwrap();
+		let offered_to_self_claim = node_txn.iter().filter(|tx| tx.input.len() == 2).next().unwrap();
+		check_spends!(accepted_claim, revoked_local_txn[0]);
+		check_spends!(offered_to_self_claim, revoked_local_txn[0]);
+		assert_eq!(accepted_claim.input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT);
 
 		let mut witness_lens = BTreeSet::new();
-		witness_lens.insert(node_txn[0].input[0].witness.last().unwrap().len());
-		witness_lens.insert(node_txn[1].input[0].witness.last().unwrap().len());
-		witness_lens.insert(node_txn[1].input[1].witness.last().unwrap().len());
-		assert_eq!(witness_lens.len(), 3);
+		witness_lens.insert(offered_to_self_claim.input[0].witness.last().unwrap().len());
+		witness_lens.insert(offered_to_self_claim.input[1].witness.last().unwrap().len());
+		assert_eq!(witness_lens.len(), 2);
 		assert_eq!(*witness_lens.iter().skip(0).next().unwrap(), 77); // revoked to_local
-		assert_eq!(*witness_lens.iter().skip(1).next().unwrap(), OFFERED_HTLC_SCRIPT_WEIGHT); // revoked offered HTLC
-		assert_eq!(*witness_lens.iter().skip(2).next().unwrap(), ACCEPTED_HTLC_SCRIPT_WEIGHT); // revoked received HTLC
+		assert_eq!(*witness_lens.iter().skip(1).next().unwrap(), OFFERED_HTLC_SCRIPT_WEIGHT);
 
-		// Finally, mine the penalty transactions and check that we get an HTLC failure after
+		// Finally, mine the penalty transaction and check that we get an HTLC failure after
 		// ANTI_REORG_DELAY confirmations.
-		mine_transaction(&nodes[1], &node_txn[0]);
-		mine_transaction(&nodes[1], &node_txn[1]);
+		mine_transaction(&nodes[1], accepted_claim);
 		connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
 		expect_payment_failed!(nodes[1], payment_hash_2, false);
 	}
@@ -5042,8 +5038,7 @@ fn test_static_spendable_outputs_timeout_tx() {
 	check_spends!(spend_txn[2], node_txn[0], commitment_tx[0]); // All outputs
 }
 
-#[test]
-fn test_static_spendable_outputs_justice_tx_revoked_commitment_tx() {
+fn do_test_static_spendable_outputs_justice_tx_revoked_commitment_tx(split_tx: bool) {
 	let chanmon_cfgs = create_chanmon_cfgs(2);
 	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
 	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
@@ -5059,20 +5054,28 @@ fn test_static_spendable_outputs_justice_tx_revoked_commitment_tx() {
 
 	claim_payment(&nodes[0], &vec!(&nodes[1])[..], payment_preimage);
 
+	if split_tx {
+		connect_blocks(&nodes[1], TEST_FINAL_CLTV - COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE + 1);
+	}
+
 	mine_transaction(&nodes[1], &revoked_local_txn[0]);
 	check_closed_broadcast!(nodes[1], true);
 	check_added_monitors!(nodes[1], 1);
 	check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed, [nodes[0].node.get_our_node_id()], 100000);
 
-	// The unpinnable, revoked to_self output and the pinnable, revoked HTLC output will be claimed
-	// in separate transactions.
+	// If the HTLC expires in more than COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE blocks, we'll
+	// claim both the revoked and HTLC outputs in one transaction, otherwise we'll split them as we
+	// consider the HTLC output as pinnable and want to claim pinnable and unpinnable outputs
+	// separately.
 	let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().clone();
-	assert_eq!(node_txn.len(), 2);
+	assert_eq!(node_txn.len(), if split_tx { 2 } else { 1 });
 	for tx in node_txn.iter() {
-		assert_eq!(tx.input.len(), 1);
+		assert_eq!(tx.input.len(), if split_tx { 1 } else { 2 });
 		check_spends!(tx, revoked_local_txn[0]);
 	}
-	assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output);
+	if split_tx {
+		assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output);
+	}
 
 	mine_transaction(&nodes[1], &node_txn[0]);
 	connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
@@ -5082,6 +5085,12 @@ fn test_static_spendable_outputs_justice_tx_revoked_commitment_tx() {
 	check_spends!(spend_txn[0], node_txn[0]);
 }
 
+#[test]
+fn test_static_spendable_outputs_justice_tx_revoked_commitment_tx() {
+	do_test_static_spendable_outputs_justice_tx_revoked_commitment_tx(true);
+	do_test_static_spendable_outputs_justice_tx_revoked_commitment_tx(false);
+}
+
 #[test]
 fn test_static_spendable_outputs_justice_tx_revoked_htlc_timeout_tx() {
 	let mut chanmon_cfgs = create_chanmon_cfgs(2);
@@ -5114,6 +5123,10 @@ fn test_static_spendable_outputs_justice_tx_revoked_htlc_timeout_tx() {
 	check_spends!(revoked_htlc_txn[0], revoked_local_txn[0]);
 	assert_ne!(revoked_htlc_txn[0].lock_time, LockTime::ZERO); // HTLC-Timeout
 
+	// In order to connect `revoked_htlc_txn[0]` we must first advance the chain by
+	// `TEST_FINAL_CLTV` blocks as otherwise the transaction is consensus-invalid due to its
+	// locktime.
+	connect_blocks(&nodes[1], TEST_FINAL_CLTV);
 	// B will generate justice tx from A's revoked commitment/HTLC tx
 	connect_block(&nodes[1], &create_dummy_block(nodes[1].best_block_hash(), 42, vec![revoked_local_txn[0].clone(), revoked_htlc_txn[0].clone()]));
 	check_closed_broadcast!(nodes[1], true);

lightning/src/ln/monitor_tests.rs

@@ -10,7 +10,7 @@
 //! Further functional tests which test blockchain reorganizations.
 
 use crate::sign::{ecdsa::EcdsaChannelSigner, OutputSpender, SpendableOutputDescriptor};
-use crate::chain::channelmonitor::{ANTI_REORG_DELAY, ARCHIVAL_DELAY_BLOCKS,LATENCY_GRACE_PERIOD_BLOCKS, Balance, BalanceSource, ChannelMonitorUpdateStep};
+use crate::chain::channelmonitor::{ANTI_REORG_DELAY, ARCHIVAL_DELAY_BLOCKS,LATENCY_GRACE_PERIOD_BLOCKS, COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE, Balance, BalanceSource, ChannelMonitorUpdateStep};
 use crate::chain::transaction::OutPoint;
 use crate::chain::chaininterface::{ConfirmationTarget, LowerBoundedFeeEstimator, compute_feerate_sat_per_1000_weight};
 use crate::events::bump_transaction::{BumpTransactionEvent, WalletSource};
@@ -1734,6 +1734,12 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
 	assert_eq!(revoked_htlc_success.lock_time, LockTime::ZERO);
 	assert_ne!(revoked_htlc_timeout.lock_time, LockTime::ZERO);
 
+	// First connect blocks until the HTLC expires with
+	// `COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE` blocks, making us consider all the HTLCs
+	// pinnable claims, which the remainder of the test assumes.
+	connect_blocks(&nodes[0], TEST_FINAL_CLTV - COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE);
+	expect_pending_htlcs_forwardable_and_htlc_handling_failed_ignore!(&nodes[0],
+		[HTLCDestination::FailedPayment { payment_hash: failed_payment_hash }]);
 	// A will generate justice tx from B's revoked commitment/HTLC tx
 	mine_transaction(&nodes[0], &revoked_local_txn[0]);
 	check_closed_broadcast!(nodes[0], true);
@@ -1846,8 +1852,6 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
 		sorted_vec(nodes[0].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances()));
 
 	connect_blocks(&nodes[0], revoked_htlc_timeout.lock_time.to_consensus_u32() - nodes[0].best_block_info().1);
-	expect_pending_htlcs_forwardable_and_htlc_handling_failed_ignore!(&nodes[0],
-		[HTLCDestination::FailedPayment { payment_hash: failed_payment_hash }]);
 	// As time goes on A may split its revocation claim transaction into multiple.
 	let as_fewer_input_rbf = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
 	for tx in as_fewer_input_rbf.iter() {