Conditional feerate-bumping of HolderCommitmentTx

We condition a CPFP triggering on observing a feerate increase
as reported by our fee estimator.

TODO: should we bump unconditionally after a timer expiration ?
Griefing vector in case of commitment tx withholding ?

Author: Antoine Riard

lightning/src/ln/channelmonitor.rs

@@ -1809,7 +1809,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 		let should_broadcast = self.would_broadcast_at_height(height, &logger);
 		if should_broadcast {
 			let holder_commitment_tx = PackageTemplate::build_holder_commitment_tx(self.funding_redeemscript.clone(), self.funding_info.0.txid.clone(), self.funding_info.0.index as u32);
-			claimable_outpoints.push(OnchainRequest { aggregation: false, bump_strategy: BumpStrategy::CPFP, feerate_previous: 0, height_timer: None, absolute_timelock: height, height_original: height, content: holder_commitment_tx });
+			claimable_outpoints.push(OnchainRequest { aggregation: false, bump_strategy: BumpStrategy::CPFP, feerate_previous: self.current_holder_commitment_tx.feerate_per_kw as u64, height_timer: None, absolute_timelock: height, height_original: height, content: holder_commitment_tx });
 		}
 		if should_broadcast {
 			self.pending_monitor_events.push(MonitorEvent::CommitmentTxBroadcasted(self.funding_info.0));

lightning/src/ln/functional_tests.rs

@@ -2744,11 +2744,12 @@ fn claim_htlc_outputs_single_tx() {
 		expect_payment_failed!(nodes[1], payment_hash_2, true);
 
 		let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
-		assert_eq!(node_txn.len(), 9);
+		assert_eq!(node_txn.len(), 13);
 		// ChannelMonitor: justice tx revoked offered htlc, justice tx revoked received htlc, justice tx revoked to_local (3)
 		// ChannelManager: local commmitment + local HTLC-timeout (2)
 		// ChannelMonitor: bumped justice tx, after one increase, bumps on HTLC aren't generated not being substantial anymore, bump on revoked to_local isn't generated due to more room for expiration (2)
 		// ChannelMonitor: local commitment + local HTLC-timeout (2)
+		// ChannelMonitor: bumped local commitment with _no_ CPFPs as feerate didn't fluctuate (3)
 
 		// Check the pair local commitment and HTLC-timeout broadcast due to HTLC expiration
 		assert_eq!(node_txn[2].input.len(), 1);
@@ -2758,6 +2759,11 @@ fn claim_htlc_outputs_single_tx() {
 		assert_eq!(witness_script.len(), OFFERED_HTLC_SCRIPT_WEIGHT); //Spending an offered htlc output
 		check_spends!(node_txn[3], node_txn[2]);
 
+		// Bumped commitments TODO check more
+		check_spends!(node_txn[10], chan_1.3);
+		check_spends!(node_txn[11], chan_1.3);
+		check_spends!(node_txn[12], chan_1.3);
+
 		// Justice transactions are indices 1-2-4
 		assert_eq!(node_txn[0].input.len(), 1);
 		assert_eq!(node_txn[1].input.len(), 1);
@@ -8431,11 +8437,12 @@ fn test_concurrent_monitor_claim() {
 	// We confirm Bob's state Y on Alice, she should broadcast a HTLC-timeout
 	watchtower_alice.simple_monitor.block_connected(&header, 136, &vec![&bob_state_y][..], &vec![]);
 	{
-		let htlc_txn = chanmon_cfgs[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
+		let txn = chanmon_cfgs[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
 		// We broadcast twice the transaction, once due to the HTLC-timeout, once due
 		// the onchain detection of the HTLC output
-		assert_eq!(htlc_txn.len(), 2);
-		check_spends!(htlc_txn[0], bob_state_y);
-		check_spends!(htlc_txn[1], bob_state_y);
+		assert_eq!(txn.len(), 3);
+		check_spends!(txn[0], bob_state_y);
+		check_spends!(txn[1], bob_state_y);
+		check_spends!(txn[2], chan_1.3);
 	}
 }

lightning/src/ln/onchain_utils.rs

@@ -582,6 +582,9 @@ impl PackageTemplate {
 
 				// We sign our commitment transaction
 				let signed_tx = onchain_handler.get_fully_signed_holder_tx(&input.1.funding_redeemscript).unwrap();
+				let mut package_txn = Vec::with_capacity(2);
+				log_trace!(logger, "Going to broadcast Holder Transaction {} claiming funding output {} from {}...", signed_tx.txid(), input.0.vout, input.0.txid);
+				package_txn.push(signed_tx);
 				let mut cpfp_tx = Transaction {
 					version: 2,
 					lock_time: 0,
@@ -591,49 +594,53 @@ impl PackageTemplate {
 						value,
 					}],
 				};
-				// TODO: make CPFP generation conditional on utxo input
-				if let Some(ref holder_tx) = onchain_handler.holder_commitment.as_ref() {
-					// We find & select our anchor output
-					let our_anchor_output_script = chan_utils::get_anchor_redeemscript(&onchain_handler.key_storage.pubkeys().funding_pubkey);
-					let mut vout = ::std::u32::MAX;
-					for (idx, outp) in holder_tx.unsigned_tx.output.iter().enumerate() {
-						if outp.script_pubkey == our_anchor_output_script.to_v0_p2wsh() {
-							vout = idx as u32;
+				// If commitment tx is still relying on its pre-signed fees to
+				// confirm, don't attach a CPFP
+				if utxo_input.is_some() {
+					if let Some(ref holder_tx) = onchain_handler.holder_commitment.as_ref() {
+						// We find & select our anchor output
+						let our_anchor_output_script = chan_utils::get_anchor_redeemscript(&onchain_handler.key_storage.pubkeys().funding_pubkey);
+						let mut vout = ::std::u32::MAX;
+						for (idx, outp) in holder_tx.unsigned_tx.output.iter().enumerate() {
+							if outp.script_pubkey == our_anchor_output_script.to_v0_p2wsh() {
+								vout = idx as u32;
+							}
 						}
-					}
-					if vout == ::std::u32::MAX { return None; }
-					let anchor_outpoint = BitcoinOutPoint {
-						txid: holder_tx.unsigned_tx.txid(),
-						vout,
-					};
-					// We take our bumping outpoint
-					let bumping_outpoint = utxo_input.as_ref().unwrap().0;
-					// We build our CPFP transaction
-					cpfp_tx.input.push(TxIn {
-						previous_output: anchor_outpoint,
-						script_sig: Script::new(),
-						sequence: 0xfffffffd,
-						witness: Vec::new(),
-					});
-					cpfp_tx.input.push(TxIn {
-						previous_output: bumping_outpoint,
-						script_sig: Script::new(),
-						sequence: 0xfffffffd,
-						witness: Vec::new(),
-					});
-					// We sign and witness finalize anchor input
-					if let Ok(anchor_sig) = onchain_handler.key_storage.sign_cpfp(&cpfp_tx, 0, ANCHOR_OUTPUT_VALUE, &onchain_handler.secp_ctx) {
-						cpfp_tx.input[0].witness.push(anchor_sig.serialize_der().to_vec());
-						cpfp_tx.input[0].witness[0].push(SigHashType::All as u8);
-						cpfp_tx.input[0].witness.push(our_anchor_output_script.into_bytes());
-					}
-					//// We sign and witness finalize bumping input
-					if let Ok(witness) = utxo_pool.provide_utxo_witness(&cpfp_tx, 1) {
-						cpfp_tx.input[1].witness = witness;
+						if vout == ::std::u32::MAX { return None; }
+						let anchor_outpoint = BitcoinOutPoint {
+							txid: holder_tx.unsigned_tx.txid(),
+							vout,
+						};
+						// We take our bumping outpoint
+						let bumping_outpoint = utxo_input.as_ref().unwrap().0;
+						// We build our CPFP transaction
+						cpfp_tx.input.push(TxIn {
+							previous_output: anchor_outpoint,
+							script_sig: Script::new(),
+							sequence: 0xfffffffd,
+							witness: Vec::new(),
+						});
+						cpfp_tx.input.push(TxIn {
+							previous_output: bumping_outpoint,
+							script_sig: Script::new(),
+							sequence: 0xfffffffd,
+							witness: Vec::new(),
+						});
+						// We sign and witness finalize anchor input
+						if let Ok(anchor_sig) = onchain_handler.key_storage.sign_cpfp(&cpfp_tx, 0, ANCHOR_OUTPUT_VALUE, &onchain_handler.secp_ctx) {
+							cpfp_tx.input[0].witness.push(anchor_sig.serialize_der().to_vec());
+							cpfp_tx.input[0].witness[0].push(SigHashType::All as u8);
+							cpfp_tx.input[0].witness.push(our_anchor_output_script.into_bytes());
+						}
+						//// We sign and witness finalize bumping input
+						if let Ok(witness) = utxo_pool.provide_utxo_witness(&cpfp_tx, 1) {
+							cpfp_tx.input[1].witness = witness;
+						}
+						log_trace!(logger, "Going to broadcast CPFP Transaction {} claiming anchor output from {}...", cpfp_tx.txid(), holder_tx.unsigned_tx.txid());
+						package_txn.push(cpfp_tx);
 					}
 				}
-				log_trace!(logger, "Going to broadcast Holder Transaction {} claiming funding output {} from {}...", signed_tx.txid(), input.0.vout, input.0.txid);
-				return Some(vec![signed_tx, cpfp_tx]);
+				return Some(package_txn);
 			}
 		}
 	}
@@ -1016,7 +1023,7 @@ pub(crate) fn compute_output_value<F: Deref, L: Deref>(predicted_weight: usize,
 	// If transaction is still relying ont its pre-committed feerate to get confirmed return
 	// a 0-value output-value as it won't be consumed further
 	if input_amounts == 0 {
-	        return Some((0, previous_feerate));
+		return Some((0, previous_feerate));
 	}
 
 	// If old feerate is 0, first iteration of this claim, use normal fee calculation

lightning/src/ln/onchaintx.rs

@@ -27,7 +27,7 @@ use ln::channelmanager::PaymentPreimage;
 use ln::chan_utils::HolderCommitmentTransaction;
 use ln::onchain_utils::{OnchainRequest, PackageTemplate, BumpStrategy};
 use ln::onchain_utils;
-use chain::chaininterface::{FeeEstimator, BroadcasterInterface};
+use chain::chaininterface::{FeeEstimator, BroadcasterInterface, ConfirmationTarget};
 use chain::keysinterface::ChannelKeys;
 use chain::utxointerface::UtxoPool;
 use util::logger::Logger;
@@ -311,7 +311,11 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
 		if cached_request.content.outpoints().len() == 0 { return None } // But don't prune pending claiming request yet, we may have to resurrect HTLCs
 
 		if cached_request.bump_strategy == BumpStrategy::CPFP {
-			cached_request.content.package_cpfp(&utxo_pool);
+			if cached_request.feerate_previous < fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::HighPriority) as u64 {
+				// Bumping UTXO is allocated the first time we detect the pre-signed feerate
+				// is our fee estimator confirmation target
+				cached_request.content.package_cpfp(&utxo_pool);
+			}
 		}
 
 		// Compute new height timer to decide when we need to regenerate a new bumped version of the claim tx (if we
@@ -332,7 +336,6 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
 			if predicted_weight == 706 || predicted_weight == 666 {
 				new_timer = None;
 			}
-
 			return Some((new_timer, new_feerate, txn))
 		}
 		None