Move HTLC tx generation in OnchainTxHandler

HTLC Transaction can't be bumped without sighash changes
so their gneeration is one-time for nwo. We move them in
OnchainTxHandler for simplifying ChannelMonitor and to prepare
storage of keys material behind one external signer interface.

Some tests break due to change in transaction broadcaster order.
Number of transactions may vary because of temporary anti-duplicata
tweak can't dissociate between 2- broadcast from different
origins (ChannelMonitor, ChannelManager) and 2-broadcast from same
component.

Author: Antoine Riard

lightning/src/ln/channelmonitor.rs

@@ -438,8 +438,6 @@ pub(crate) enum InputMaterial {
 		locktime: u32,
 	},
 	LocalHTLC {
-		witness_script: Script,
-		sigs: (Signature, Signature),
 		preimage: Option<PaymentPreimage>,
 		amount: u64,
 	},
@@ -467,11 +465,8 @@ impl Writeable for InputMaterial  {
 				writer.write_all(&byte_utils::be64_to_array(*amount))?;
 				writer.write_all(&byte_utils::be32_to_array(*locktime))?;
 			},
-			&InputMaterial::LocalHTLC { ref witness_script, ref sigs, ref preimage, ref amount } => {
+			&InputMaterial::LocalHTLC { ref preimage, ref amount } => {
 				writer.write_all(&[2; 1])?;
-				witness_script.write(writer)?;
-				sigs.0.write(writer)?;
-				sigs.1.write(writer)?;
 				preimage.write(writer)?;
 				writer.write_all(&byte_utils::be64_to_array(*amount))?;
 			},
@@ -516,16 +511,11 @@ impl Readable for InputMaterial {
 				}
 			},
 			2 => {
-				let witness_script = Readable::read(reader)?;
-				let their_sig = Readable::read(reader)?;
-				let our_sig = Readable::read(reader)?;
 				let preimage = Readable::read(reader)?;
 				let amount = Readable::read(reader)?;
 				InputMaterial::LocalHTLC {
-					witness_script,
-					sigs: (their_sig, our_sig),
 					preimage,
-					amount
+					amount,
 				}
 			},
 			3 => {
@@ -1679,49 +1669,32 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 		(claimable_outpoints, Some((htlc_txid, tx.output.clone())))
 	}
 
-	fn broadcast_by_local_state(&self, commitment_tx: &Transaction, local_tx: &LocalSignedTx) -> (Vec<Transaction>, Vec<TxOut>, Option<(Script, SecretKey, Script)>) {
-		let mut res = Vec::with_capacity(local_tx.htlc_outputs.len());
+	fn broadcast_by_local_state(&self, commitment_tx: &Transaction, local_tx: &LocalSignedTx) -> (Vec<ClaimRequest>, Vec<TxOut>, Option<(Script, SecretKey, Script)>) {
+		let mut claim_requests = Vec::with_capacity(local_tx.htlc_outputs.len());
 		let mut watch_outputs = Vec::with_capacity(local_tx.htlc_outputs.len());
 
 		let redeemscript = chan_utils::get_revokeable_redeemscript(&local_tx.revocation_key, self.their_to_self_delay.unwrap(), &local_tx.delayed_payment_key);
 		let broadcasted_local_revokable_script = if let Ok(local_delayedkey) = chan_utils::derive_private_key(&self.secp_ctx, &local_tx.per_commitment_point, self.onchain_detection.keys.delayed_payment_base_key()) {
 			Some((redeemscript.to_v0_p2wsh(), local_delayedkey, redeemscript))
 		} else { None };
 
-		for &(ref htlc, ref sigs, _) in local_tx.htlc_outputs.iter() {
+		for &(ref htlc, _, _) in local_tx.htlc_outputs.iter() {
 			if let Some(transaction_output_index) = htlc.transaction_output_index {
-				if let &Some(ref their_sig) = sigs {
-					if htlc.offered {
-						log_trace!(self, "Broadcasting HTLC-Timeout transaction against local commitment transactions");
-						let mut htlc_timeout_tx = chan_utils::build_htlc_transaction(&local_tx.txid, local_tx.feerate_per_kw, self.their_to_self_delay.unwrap(), htlc, &local_tx.delayed_payment_key, &local_tx.revocation_key);
-						self.onchain_detection.keys.sign_htlc_transaction(&mut htlc_timeout_tx, their_sig, &None, htlc, &local_tx.a_htlc_key, &local_tx.b_htlc_key, &local_tx.revocation_key, &local_tx.per_commitment_point, &self.secp_ctx);
-
-						log_trace!(self, "Outpoint {}:{} is being being claimed", htlc_timeout_tx.input[0].previous_output.vout, htlc_timeout_tx.input[0].previous_output.txid);
-						res.push(htlc_timeout_tx);
-					} else {
-						if let Some(payment_preimage) = self.payment_preimages.get(&htlc.payment_hash) {
-							log_trace!(self, "Broadcasting HTLC-Success transaction against local commitment transactions");
-							let mut htlc_success_tx = chan_utils::build_htlc_transaction(&local_tx.txid, local_tx.feerate_per_kw, self.their_to_self_delay.unwrap(), htlc, &local_tx.delayed_payment_key, &local_tx.revocation_key);
-							self.onchain_detection.keys.sign_htlc_transaction(&mut htlc_success_tx, their_sig, &Some(*payment_preimage), htlc, &local_tx.a_htlc_key, &local_tx.b_htlc_key, &local_tx.revocation_key, &local_tx.per_commitment_point, &self.secp_ctx);
-
-							log_trace!(self, "Outpoint {}:{} is being being claimed", htlc_success_tx.input[0].previous_output.vout, htlc_success_tx.input[0].previous_output.txid);
-							res.push(htlc_success_tx);
-						}
-					}
-					watch_outputs.push(commitment_tx.output[transaction_output_index as usize].clone());
-				} else { panic!("Should have sigs for non-dust local tx outputs!") }
+				let preimage = if let Some(preimage) = self.payment_preimages.get(&htlc.payment_hash) { Some(*preimage) } else { None };
+				claim_requests.push(ClaimRequest { absolute_timelock: ::std::u32::MAX, aggregable: false, outpoint: BitcoinOutPoint { txid: local_tx.txid, vout: transaction_output_index as u32 }, witness_data: InputMaterial::LocalHTLC { preimage, amount: htlc.amount_msat / 1000 }});
+				watch_outputs.push(commitment_tx.output[transaction_output_index as usize].clone());
 			}
 		}
 
-		(res, watch_outputs, broadcasted_local_revokable_script)
+		(claim_requests, watch_outputs, broadcasted_local_revokable_script)
 	}
 
 	/// Attempts to claim any claimable HTLCs in a commitment transaction which was not (yet)
 	/// revoked using data in local_claimable_outpoints.
 	/// Should not be used if check_spend_revoked_transaction succeeds.
-	fn check_spend_local_transaction(&mut self, tx: &Transaction, height: u32) -> (Vec<Transaction>, (Sha256dHash, Vec<TxOut>)) {
+	fn check_spend_local_transaction(&mut self, tx: &Transaction, height: u32) -> (Vec<ClaimRequest>, (Sha256dHash, Vec<TxOut>)) {
 		let commitment_txid = tx.txid();
-		let mut local_txn = Vec::new();
+		let mut claim_requests = Vec::new();
 		let mut watch_outputs = Vec::new();
 
 		macro_rules! wait_threshold_conf {
@@ -1749,7 +1722,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 
 		macro_rules! append_onchain_update {
 			($updates: expr) => {
-				local_txn.append(&mut $updates.0);
+				claim_requests = $updates.0;
 				watch_outputs.append(&mut $updates.1);
 				self.broadcasted_local_revokable_script = $updates.2;
 			}
@@ -1796,7 +1769,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 			}
 		}
 
-		(local_txn, (commitment_txid, watch_outputs))
+		(claim_requests, (commitment_txid, watch_outputs))
 	}
 
 	/// Used by ChannelManager deserialization to broadcast the latest local state if its copy of
@@ -1866,14 +1839,11 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 							watch_outputs.push(new_outputs);
 						}
 						if new_outpoints.is_empty() {
-							let (local_txn, new_outputs) = self.check_spend_local_transaction(&tx, height);
-							for tx in local_txn.iter() {
-								log_trace!(self, "Broadcast onchain {}", log_tx!(tx));
-								broadcaster.broadcast_transaction(tx);
-							}
+							let (mut new_outpoints, new_outputs) = self.check_spend_local_transaction(&tx, height);
 							if !new_outputs.1.is_empty() {
 								watch_outputs.push(new_outputs);
 							}
+							claimable_outpoints.append(&mut new_outpoints);
 						}
 						claimable_outpoints.append(&mut new_outpoints);
 					}
@@ -1903,14 +1873,11 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 		if let Some(ref cur_local_tx) = self.current_local_signed_commitment_tx {
 			if should_broadcast {
 				if let Some(commitment_tx) = self.onchain_tx_handler.get_fully_signed_local_tx(self.channel_value_satoshis.unwrap()) {
-					let (txs, new_outputs, _) = self.broadcast_by_local_state(&commitment_tx, cur_local_tx);
+					let (mut new_outpoints, new_outputs, _) = self.broadcast_by_local_state(&commitment_tx, cur_local_tx);
 					if !new_outputs.is_empty() {
 						watch_outputs.push((cur_local_tx.txid.clone(), new_outputs));
 					}
-					for tx in txs {
-						log_trace!(self, "Broadcast onchain {}", log_tx!(tx));
-						broadcaster.broadcast_transaction(&tx);
-					}
+					claimable_outpoints.append(&mut new_outpoints);
 				}
 			}
 		}

lightning/src/ln/functional_tests.rs

@@ -2353,16 +2353,16 @@ fn claim_htlc_outputs_single_tx() {
 		// ChannelMonitor: local commitment + local HTLC-timeout (2)
 
 		// Check the pair local commitment and HTLC-timeout broadcast due to HTLC expiration
+		assert_eq!(node_txn[2].input.len(), 1);
+		check_spends!(node_txn[2], chan_1.3.clone());
 		assert_eq!(node_txn[3].input.len(), 1);
-		check_spends!(node_txn[3], chan_1.3.clone());
-		assert_eq!(node_txn[0].input.len(), 1);
-		let witness_script = node_txn[0].input[0].witness.last().unwrap();
+		let witness_script = node_txn[3].input[0].witness.last().unwrap();
 		assert_eq!(witness_script.len(), OFFERED_HTLC_SCRIPT_WEIGHT); //Spending an offered htlc output
-		check_spends!(node_txn[0], node_txn[3].clone());
+		check_spends!(node_txn[3], node_txn[2].clone());
 
 		// Justice transactions are indices 1-2-4
+		assert_eq!(node_txn[0].input.len(), 1);
 		assert_eq!(node_txn[1].input.len(), 1);
-		assert_eq!(node_txn[2].input.len(), 1);
 		assert_eq!(node_txn[4].input.len(), 1);
 
 		fn get_txout(out_point: &BitcoinOutPoint, tx: &Transaction) -> Option<TxOut> {
@@ -2372,13 +2372,13 @@ fn claim_htlc_outputs_single_tx() {
 				None
 			}
 		}
+		node_txn[0].verify(|out|get_txout(out, &revoked_local_txn[0])).unwrap();
 		node_txn[1].verify(|out|get_txout(out, &revoked_local_txn[0])).unwrap();
-		node_txn[2].verify(|out|get_txout(out, &revoked_local_txn[0])).unwrap();
 		node_txn[4].verify(|out|get_txout(out, &revoked_local_txn[0])).unwrap();
 
 		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[2].input[0].witness.last().unwrap().len());
 		witness_lens.insert(node_txn[4].input[0].witness.last().unwrap().len());
 		assert_eq!(witness_lens.len(), 3);
 		assert_eq!(*witness_lens.iter().skip(0).next().unwrap(), 77); // revoked to_local
@@ -2630,19 +2630,18 @@ fn test_htlc_on_chain_timeout() {
 	{
 		let mut node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
 		assert_eq!(node_txn.len(), 5); // ChannelManager : 2 (commitment tx, HTLC-Timeout tx), ChannelMonitor : (local commitment tx + HTLC-timeout), timeout tx
+		assert_eq!(node_txn[1], node_txn[3]);
+		assert_eq!(node_txn[2], node_txn[4]);
 
-		assert_eq!(node_txn[2], node_txn[3]);
-		assert_eq!(node_txn[0], node_txn[4]);
-
-		check_spends!(node_txn[1], commitment_tx[0]);
-		assert_eq!(node_txn[1].clone().input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT);
+		check_spends!(node_txn[0], commitment_tx[0]);
+		assert_eq!(node_txn[0].clone().input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT);
 
-		check_spends!(node_txn[2], chan_2.3);
-		check_spends!(node_txn[0], node_txn[2]);
-		assert_eq!(node_txn[2].clone().input[0].witness.last().unwrap().len(), 71);
-		assert_eq!(node_txn[0].clone().input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT);
+		check_spends!(node_txn[1], chan_2.3);
+		check_spends!(node_txn[2], node_txn[1]);
+		assert_eq!(node_txn[1].clone().input[0].witness.last().unwrap().len(), 71);
+		assert_eq!(node_txn[2].clone().input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT);
 
-		timeout_tx = node_txn[1].clone();
+		timeout_tx = node_txn[0].clone();
 		node_txn.clear();
 	}
 
@@ -4300,8 +4299,7 @@ fn test_static_spendable_outputs_justice_tx_revoked_htlc_timeout_tx() {
 	check_added_monitors!(nodes[0], 1);
 
 	let revoked_htlc_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
-	assert_eq!(revoked_htlc_txn.len(), 3);
-	assert_eq!(revoked_htlc_txn[0], revoked_htlc_txn[2]);
+	assert_eq!(revoked_htlc_txn.len(), 2);
 	assert_eq!(revoked_htlc_txn[0].input.len(), 1);
 	assert_eq!(revoked_htlc_txn[0].input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT);
 	check_spends!(revoked_htlc_txn[0], revoked_local_txn[0]);
@@ -4357,8 +4355,7 @@ fn test_static_spendable_outputs_justice_tx_revoked_htlc_success_tx() {
 	check_added_monitors!(nodes[1], 1);
 	let revoked_htlc_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
 
-	assert_eq!(revoked_htlc_txn.len(), 3);
-	assert_eq!(revoked_htlc_txn[0], revoked_htlc_txn[2]);
+	assert_eq!(revoked_htlc_txn.len(), 2);
 	assert_eq!(revoked_htlc_txn[0].input.len(), 1);
 	assert_eq!(revoked_htlc_txn[0].input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT);
 	check_spends!(revoked_htlc_txn[0], revoked_local_txn[0]);
@@ -7087,7 +7084,7 @@ fn test_bump_penalty_txn_on_revoked_htlcs() {
 	check_added_monitors!(nodes[1], 1);
 
 	let revoked_htlc_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
-	assert_eq!(revoked_htlc_txn.len(), 6);
+	assert_eq!(revoked_htlc_txn.len(), 4);
 	if revoked_htlc_txn[0].input[0].witness.last().unwrap().len() == ACCEPTED_HTLC_SCRIPT_WEIGHT {
 		assert_eq!(revoked_htlc_txn[0].input.len(), 1);
 		check_spends!(revoked_htlc_txn[0], revoked_local_txn[0]);
@@ -7345,11 +7342,11 @@ fn test_set_outpoints_partial_claiming() {
 	let partial_claim_tx = {
 		let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
 		assert_eq!(node_txn.len(), 3);
-		check_spends!(node_txn[0], node_txn[2].clone());
-		check_spends!(node_txn[1], node_txn[2].clone());
-		assert_eq!(node_txn[0].input.len(), 1);
+		check_spends!(node_txn[1], node_txn[0].clone());
+		check_spends!(node_txn[2], node_txn[0].clone());
 		assert_eq!(node_txn[1].input.len(), 1);
-		node_txn[0].clone()
+		assert_eq!(node_txn[2].input.len(), 1);
+		node_txn[1].clone()
 	};
 
 	// Broadcast partial claim on node A, should regenerate a claiming tx with HTLC dropped

lightning/src/ln/onchaintx.rs

@@ -530,13 +530,14 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
 					inputs_witnesses_weight += Self::get_witnesses_weight(if preimage.is_some() { &[InputDescriptors::OfferedHTLC] } else { &[InputDescriptors::ReceivedHTLC] });
 					amt += *amount;
 				},
-				&InputMaterial::LocalHTLC { .. } => { return None; }
+				&InputMaterial::LocalHTLC { .. } => {
+					dynamic_fee = false;
+				},
 				&InputMaterial::Funding { .. } => {
 					dynamic_fee = false;
 				}
 			}
 		}
-
 		if dynamic_fee {
 			let predicted_weight = bumped_tx.get_weight() + inputs_witnesses_weight;
 			let mut new_feerate;
@@ -598,11 +599,28 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
 		} else {
 			for (_, (outp, per_outp_material)) in cached_claim_datas.per_input_material.iter().enumerate() {
 				match per_outp_material {
-					&InputMaterial::LocalHTLC { .. } => {
-						//TODO : Given that Local Commitment Transaction and HTLC-Timeout/HTLC-Success are counter-signed by peer, we can't
-						// RBF them. Need a Lightning specs change and package relay modification :
-						// https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html
-						return None;
+					&InputMaterial::LocalHTLC { ref preimage, ref amount } => {
+						let this_htlc_cache = if let Some(ref local_commitment) = self.local_commitment {
+							if outp.txid == local_commitment.txid() {
+								&self.current_htlc_cache
+							} else if let Some(ref prev_commitment) = self.prev_local_commitment {
+								if outp.txid == prev_commitment.txid() { &self.prev_htlc_cache } else { &None }
+							} else { &None }
+						} else { &None };
+						if let &Some(ref htlc_cache) = this_htlc_cache {
+							if let Some(htlc) = htlc_cache.per_htlc.get(&outp.vout) {
+								if !htlc.0.offered && preimage.is_none() { return None; }; // If we don't have preimage for HTLC-Success, don't try to generate
+								let htlc_secret = if !htlc.0.offered { preimage } else { &None }; // If we have a preimage for a HTLC-Timeout, don't use it that's likely a duplicate HTLC hash
+
+								let mut htlc_tx = chan_utils::build_htlc_transaction(&outp.txid, htlc_cache.feerate_per_kw, self.local_csv, &htlc.0, &htlc_cache.local_keys.a_delayed_payment_key, &htlc_cache.local_keys.revocation_key);
+								self.key_storage.sign_htlc_transaction(&mut htlc_tx, htlc.1.as_ref().unwrap(), htlc_secret, &htlc.0, &htlc_cache.local_keys.a_htlc_key, &htlc_cache.local_keys.b_htlc_key, &htlc_cache.local_keys.revocation_key, &htlc_cache.local_keys.per_commitment_point, &self.secp_ctx);
+
+								let feerate = (amount - htlc_tx.output[0].value) * 1000 / htlc_tx.get_weight() as u64;
+								// Timer set to $NEVER given we can't bump tx without anchor outputs
+								log_trace!(self, "Going to broadcast Local HTLC-{} claiming HTLC output {} from {}...", if preimage.is_some() { "Success" } else { "Timeout" }, outp.vout, outp.txid);
+								return Some((None, feerate, htlc_tx));
+							}
+						}
 					},
 					&InputMaterial::Funding { ref channel_value } => {
 						if self.local_commitment.is_some() {