Avoid commitment broadcast upon detected funding spend

There's no need to broadcast our local commitment transaction if we've
already seen a confirmed one as it'll be immediately rejected as a
duplicate/conflict.

This will also help prevent dispatching spurious events for bumping
commitment and HTLC transactions through anchor outputs (once
implemented in future work) and the dispatch for said events follows the
same flow as our usual commitment broadcast.

Author: wpaulino

lightning/src/chain/channelmonitor.rs

@@ -3042,6 +3042,16 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 	}
 
 	fn should_broadcast_holder_commitment_txn<L: Deref>(&self, logger: &L) -> bool where L::Target: Logger {
+		// There's no need to broadcast our commitment transaction if we've seen one confirmed (even
+		// with 1 confirmation) as it'll be rejected as duplicate/conflicting.
+		//
+		// TODO: This is currently never reset to false if the spend seen is unconfirmed, which will
+		// cause us to not broadcast even if we have an HTLC to claim on-chain. However, resetting
+		// it to false may have unintended side-effects on channel monitor updates. Perhaps we
+		// should introduce a new struct member for our broadcast purpose?
+		if self.funding_spend_seen {
+			return false;
+		}
 		// We need to consider all HTLCs which are:
 		//  * in any unrevoked counterparty commitment transaction, as they could broadcast said
 		//    transactions and we'd end up in a race, or

lightning/src/ln/functional_tests.rs

@@ -1267,44 +1267,32 @@ fn test_duplicate_htlc_different_direction_onchain() {
 	connect_blocks(&nodes[0], TEST_FINAL_CLTV - 1); // Confirm blocks until the HTLC expires
 
 	let claim_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().clone();
-	assert_eq!(claim_txn.len(), 8);
+	assert_eq!(claim_txn.len(), 5);
 
 	check_spends!(claim_txn[0], remote_txn[0]); // Immediate HTLC claim with preimage
-
 	check_spends!(claim_txn[1], chan_1.3); // Alternative commitment tx
 	check_spends!(claim_txn[2], claim_txn[1]); // HTLC spend in alternative commitment tx
 
-	let bump_tx = if claim_txn[1] == claim_txn[4] {
-		assert_eq!(claim_txn[1], claim_txn[4]);
-		assert_eq!(claim_txn[2], claim_txn[5]);
-
-		check_spends!(claim_txn[7], claim_txn[1]); // HTLC timeout on alternative commitment tx
-
-		check_spends!(claim_txn[3], remote_txn[0]); // HTLC timeout on broadcasted commitment tx
-		&claim_txn[3]
+	check_spends!(claim_txn[3], remote_txn[0]);
+	check_spends!(claim_txn[4], remote_txn[0]);
+	let preimage_tx = &claim_txn[0];
+	let (preimage_bump_tx, timeout_tx) = if claim_txn[3].input[0].previous_output == preimage_tx.input[0].previous_output {
+		(&claim_txn[3], &claim_txn[4])
 	} else {
-		assert_eq!(claim_txn[1], claim_txn[3]);
-		assert_eq!(claim_txn[2], claim_txn[4]);
-
-		check_spends!(claim_txn[5], claim_txn[1]); // HTLC timeout on alternative commitment tx
-
-		check_spends!(claim_txn[7], remote_txn[0]); // HTLC timeout on broadcasted commitment tx
-
-		&claim_txn[7]
+		(&claim_txn[4], &claim_txn[3])
 	};
 
-	assert_eq!(claim_txn[0].input.len(), 1);
-	assert_eq!(bump_tx.input.len(), 1);
-	assert_eq!(claim_txn[0].input[0].previous_output, bump_tx.input[0].previous_output);
+	assert_eq!(preimage_tx.input.len(), 1);
+	assert_eq!(preimage_bump_tx.input.len(), 1);
 
-	assert_eq!(claim_txn[0].input.len(), 1);
-	assert_eq!(claim_txn[0].input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT); // HTLC 1 <--> 0, preimage tx
-	assert_eq!(remote_txn[0].output[claim_txn[0].input[0].previous_output.vout as usize].value, 800);
+	assert_eq!(preimage_tx.input.len(), 1);
+	assert_eq!(preimage_tx.input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT); // HTLC 1 <--> 0, preimage tx
+	assert_eq!(remote_txn[0].output[preimage_tx.input[0].previous_output.vout as usize].value, 800);
 
-	assert_eq!(claim_txn[6].input.len(), 1);
-	assert_eq!(claim_txn[6].input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT); // HTLC 0 <--> 1, timeout tx
-	check_spends!(claim_txn[6], remote_txn[0]);
-	assert_eq!(remote_txn[0].output[claim_txn[6].input[0].previous_output.vout as usize].value, 900);
+	assert_eq!(timeout_tx.input.len(), 1);
+	assert_eq!(timeout_tx.input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT); // HTLC 0 <--> 1, timeout tx
+	check_spends!(timeout_tx, remote_txn[0]);
+	assert_eq!(remote_txn[0].output[timeout_tx.input[0].previous_output.vout as usize].value, 900);
 
 	let events = nodes[0].node.get_and_clear_pending_msg_events();
 	assert_eq!(events.len(), 3);
@@ -8036,45 +8024,40 @@ fn test_bump_penalty_txn_on_remote_commitment() {
 	let feerate_preimage;
 	{
 		let mut node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
-		// 9 transactions including:
-		// 1*2 ChannelManager local broadcasts of commitment + HTLC-Success
-		// 1*3 ChannelManager local broadcasts of commitment + HTLC-Success + HTLC-Timeout
-		// 2 * HTLC-Success (one RBF bump we'll check later)
-		// 1 * HTLC-Timeout
-		assert_eq!(node_txn.len(), 8);
+		// 5 transactions including:
+		//   local commitment + HTLC-Success
+		//   preimage and timeout sweeps from remote commitment + preimage sweep bump
+		assert_eq!(node_txn.len(), 5);
 		assert_eq!(node_txn[0].input.len(), 1);
-		assert_eq!(node_txn[6].input.len(), 1);
+		assert_eq!(node_txn[3].input.len(), 1);
+		assert_eq!(node_txn[4].input.len(), 1);
 		check_spends!(node_txn[0], remote_txn[0]);
-		check_spends!(node_txn[6], remote_txn[0]);
-
-		check_spends!(node_txn[1], chan.3);
-		check_spends!(node_txn[2], node_txn[1]);
-
-		if node_txn[0].input[0].previous_output == node_txn[3].input[0].previous_output {
-			preimage_bump = node_txn[3].clone();
-			check_spends!(node_txn[3], remote_txn[0]);
+		check_spends!(node_txn[3], remote_txn[0]);
+		check_spends!(node_txn[4], remote_txn[0]);
 
-			assert_eq!(node_txn[1], node_txn[4]);
-			assert_eq!(node_txn[2], node_txn[5]);
-		} else {
-			preimage_bump = node_txn[7].clone();
-			check_spends!(node_txn[7], remote_txn[0]);
-			assert_eq!(node_txn[0].input[0].previous_output, node_txn[7].input[0].previous_output);
-
-			assert_eq!(node_txn[1], node_txn[3]);
-			assert_eq!(node_txn[2], node_txn[4]);
-		}
-
-		timeout = node_txn[6].txid();
-		let index = node_txn[6].input[0].previous_output.vout;
-		let fee = remote_txn[0].output[index as usize].value - node_txn[6].output[0].value;
-		feerate_timeout = fee * 1000 / node_txn[6].weight() as u64;
+		check_spends!(node_txn[1], chan.3); // local commitment
+		check_spends!(node_txn[2], node_txn[1]); // local HTLC-Success
 
 		preimage = node_txn[0].txid();
 		let index = node_txn[0].input[0].previous_output.vout;
 		let fee = remote_txn[0].output[index as usize].value - node_txn[0].output[0].value;
 		feerate_preimage = fee * 1000 / node_txn[0].weight() as u64;
 
+		let (preimage_bump_tx, timeout_tx) = if node_txn[3].input[0].previous_output == node_txn[0].input[0].previous_output {
+			(node_txn[3].clone(), node_txn[4].clone())
+		} else {
+			(node_txn[4].clone(), node_txn[3].clone())
+		};
+
+		preimage_bump = preimage_bump_tx;
+		check_spends!(preimage_bump, remote_txn[0]);
+		assert_eq!(node_txn[0].input[0].previous_output, preimage_bump.input[0].previous_output);
+
+		timeout = timeout_tx.txid();
+		let index = timeout_tx.input[0].previous_output.vout;
+		let fee = remote_txn[0].output[index as usize].value - timeout_tx.output[0].value;
+		feerate_timeout = fee * 1000 / timeout_tx.weight() as u64;
+
 		node_txn.clear();
 	};
 	assert_ne!(feerate_timeout, 0);
@@ -8950,11 +8933,8 @@ fn test_concurrent_monitor_claim() {
 	watchtower_alice.chain_monitor.block_connected(&Block { header, txdata: vec![bob_state_y.clone()] }, CHAN_CONFIRM_DEPTH + 2 + TEST_FINAL_CLTV + LATENCY_GRACE_PERIOD_BLOCKS);
 	{
 		let htlc_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);
+		assert_eq!(htlc_txn.len(), 1);
 		check_spends!(htlc_txn[0], bob_state_y);
-		check_spends!(htlc_txn[1], bob_state_y);
 	}
 }
 

lightning/src/ln/payment_tests.rs

@@ -508,13 +508,8 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) {
 	expect_payment_sent!(nodes[0], payment_preimage_1);
 	connect_blocks(&nodes[0], TEST_FINAL_CLTV*4 + 20);
 	let as_htlc_timeout_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
-	assert_eq!(as_htlc_timeout_txn.len(), 3);
-	let (first_htlc_timeout_tx, second_htlc_timeout_tx) = if as_htlc_timeout_txn[0] == as_commitment_tx {
-		(&as_htlc_timeout_txn[1], &as_htlc_timeout_txn[2])
-	} else {
-		assert_eq!(as_htlc_timeout_txn[2], as_commitment_tx);
-		(&as_htlc_timeout_txn[0], &as_htlc_timeout_txn[1])
-	};
+	assert_eq!(as_htlc_timeout_txn.len(), 2);
+	let (first_htlc_timeout_tx, second_htlc_timeout_tx) = (&as_htlc_timeout_txn[0], &as_htlc_timeout_txn[1]);
 	check_spends!(first_htlc_timeout_tx, as_commitment_tx);
 	check_spends!(second_htlc_timeout_tx, as_commitment_tx);
 	if first_htlc_timeout_tx.input[0].previous_output == bs_htlc_claim_txn[0].input[0].previous_output {