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

@@ -765,7 +765,10 @@ pub(crate) struct ChannelMonitorImpl<Signer: Sign> {
 	// of block connection between ChannelMonitors and the ChannelManager.
 	funding_spend_seen: bool,
 
+	/// Set to `Some` of the confirmed transaction spending the funding input of the channel after
+	/// reaching `ANTI_REORG_DELAY` confirmations.
 	funding_spend_confirmed: Option<Txid>,
+
 	confirmed_commitment_tx_counterparty_output: CommitmentTxCounterpartyOutputInfo,
 	/// The set of HTLCs which have been either claimed or failed on chain and have reached
 	/// the requisite confirmations on the claim/fail transaction (either ANTI_REORG_DELAY or the
@@ -3042,6 +3045,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.
+		if self.funding_spend_confirmed.is_some() ||
+			self.onchain_events_awaiting_threshold_conf.iter().find(|event| match event.event {
+				OnchainEvent::FundingSpendConfirmation { .. } => true,
+				_ => false,
+			}).is_some()
+		{
+			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 {