Merge pull request #1461 from TheBlueMatt/2022-05-unconf-0-not-half

Force-close channels on reorg only if the funding is unconfirmed

Author: valentinewallace

lightning/src/ln/channel.rs

@@ -4739,10 +4739,14 @@ impl<Signer: Sign> Channel<Signer> {
 			}
 
 			// If we've sent funding_locked (or have both sent and received funding_locked), and
-			// the funding transaction's confirmation count has dipped below minimum_depth / 2,
+			// the funding transaction has become unconfirmed,
 			// close the channel and hope we can get the latest state on chain (because presumably
 			// the funding transaction is at least still in the mempool of most nodes).
-			if funding_tx_confirmations < self.minimum_depth.unwrap() as i64 / 2 {
+			//
+			// Note that ideally we wouldn't force-close if we see *any* reorg on a 1-conf channel,
+			// but not doing so may lead to the `ChannelManager::short_to_id` map being
+			// inconsistent, so we currently have to.
+			if funding_tx_confirmations == 0 && self.funding_tx_confirmed_in.is_some() {
 				let err_reason = format!("Funding transaction was un-confirmed. Locked at {} confs, now have {} confs.",
 					self.minimum_depth.unwrap(), funding_tx_confirmations);
 				return Err(ClosureReason::ProcessingError { err: err_reason });

lightning/src/ln/reorg_tests.rs

@@ -209,14 +209,14 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
 			let relevant_txids = nodes[0].node.get_relevant_txids();
 			assert_eq!(&relevant_txids[..], &[chan.3.txid()]);
 			nodes[0].node.transaction_unconfirmed(&relevant_txids[0]);
+		} else if connect_style == ConnectStyle::FullBlockViaListen {
+			disconnect_blocks(&nodes[0], CHAN_CONFIRM_DEPTH - 1);
+			assert_eq!(nodes[0].node.list_usable_channels().len(), 1);
+			disconnect_blocks(&nodes[0], 1);
 		} else {
 			disconnect_all_blocks(&nodes[0]);
 		}
-		if connect_style == ConnectStyle::FullBlockViaListen && !use_funding_unconfirmed {
-			handle_announce_close_broadcast_events(&nodes, 0, 1, true, "Channel closed because of an exception: Funding transaction was un-confirmed. Locked at 6 confs, now have 2 confs.");
-		} else {
-			handle_announce_close_broadcast_events(&nodes, 0, 1, true, "Channel closed because of an exception: Funding transaction was un-confirmed. Locked at 6 confs, now have 0 confs.");
-		}
+		handle_announce_close_broadcast_events(&nodes, 0, 1, true, "Channel closed because of an exception: Funding transaction was un-confirmed. Locked at 6 confs, now have 0 confs.");
 		check_added_monitors!(nodes[1], 1);
 		{
 			let channel_state = nodes[0].node.channel_state.lock().unwrap();
@@ -277,14 +277,14 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
 			let relevant_txids = nodes[0].node.get_relevant_txids();
 			assert_eq!(&relevant_txids[..], &[chan.3.txid()]);
 			nodes[0].node.transaction_unconfirmed(&relevant_txids[0]);
+		} else if connect_style == ConnectStyle::FullBlockViaListen {
+			disconnect_blocks(&nodes[0], CHAN_CONFIRM_DEPTH - 1);
+			assert_eq!(nodes[0].node.list_channels().len(), 1);
+			disconnect_blocks(&nodes[0], 1);
 		} else {
 			disconnect_all_blocks(&nodes[0]);
 		}
-		if connect_style == ConnectStyle::FullBlockViaListen && !use_funding_unconfirmed {
-			handle_announce_close_broadcast_events(&nodes, 0, 1, true, "Channel closed because of an exception: Funding transaction was un-confirmed. Locked at 6 confs, now have 2 confs.");
-		} else {
-			handle_announce_close_broadcast_events(&nodes, 0, 1, true, "Channel closed because of an exception: Funding transaction was un-confirmed. Locked at 6 confs, now have 0 confs.");
-		}
+		handle_announce_close_broadcast_events(&nodes, 0, 1, true, "Channel closed because of an exception: Funding transaction was un-confirmed. Locked at 6 confs, now have 0 confs.");
 		check_added_monitors!(nodes[1], 1);
 		{
 			let channel_state = nodes[0].node.channel_state.lock().unwrap();
@@ -297,11 +297,7 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
 	*nodes[0].chain_monitor.expect_channel_force_closed.lock().unwrap() = Some((chan.2, true));
 	nodes[0].node.test_process_background_events(); // Required to free the pending background monitor update
 	check_added_monitors!(nodes[0], 1);
-	let expected_err = if connect_style == ConnectStyle::FullBlockViaListen && !use_funding_unconfirmed {
-		"Funding transaction was un-confirmed. Locked at 6 confs, now have 2 confs."
-	} else {
-		"Funding transaction was un-confirmed. Locked at 6 confs, now have 0 confs."
-	};
+	let expected_err = "Funding transaction was un-confirmed. Locked at 6 confs, now have 0 confs.";
 	check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: "Channel closed because of an exception: ".to_owned() + expected_err });
 	check_closed_event!(nodes[0], 1, ClosureReason::ProcessingError { err: expected_err.to_owned() });
 	assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().len(), 1);