Merge pull request #750 from TheBlueMatt/2020-11-dup-chan-id-crash

Do not generate a channel-closed mon update for never-signed chans

Author: TheBlueMatt

lightning/src/chain/chainmonitor.rs

@@ -194,6 +194,13 @@ where C::Target: chain::Filter,
 		match monitors.get_mut(&funding_txo) {
 			None => {
 				log_error!(self.logger, "Failed to update channel monitor: no such monitor registered");
+
+				// We should never ever trigger this from within ChannelManager. Technically a
+				// user could use this object with some proxying in between which makes this
+				// possible, but in tests and fuzzing, this should be a panic.
+				#[cfg(any(test, feature = "fuzztarget"))]
+				panic!("ChannelManager generated a channel update for a channel that was not yet registered!");
+				#[cfg(not(any(test, feature = "fuzztarget")))]
 				Err(ChannelMonitorUpdateErr::PermanentFailure)
 			},
 			Some(orig_monitor) => {

lightning/src/ln/channel.rs

@@ -4016,11 +4016,23 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 				_ => {}
 			}
 		}
+		let funding_txo = if let Some(funding_txo) = self.funding_txo {
+			// If we haven't yet exchanged funding signatures (ie channel_state < FundingSent),
+			// returning a channel monitor update here would imply a channel monitor update before
+			// we even registered the channel monitor to begin with, which is invalid.
+			// Thus, if we aren't actually at a point where we could conceivably broadcast the
+			// funding transaction, don't return a funding txo (which prevents providing the
+			// monitor update to the user, even if we return one).
+			// See test_duplicate_chan_id and test_pre_lockin_no_chan_closed_update for more.
+			if self.channel_state & (ChannelState::FundingSent as u32 | ChannelState::ChannelFunded as u32 | ChannelState::ShutdownComplete as u32) != 0 {
+				Some(funding_txo.clone())
+			} else { None }
+		} else { None };
 
 		self.channel_state = ChannelState::ShutdownComplete as u32;
 		self.update_time_counter += 1;
 		self.latest_monitor_update_id += 1;
-		(self.funding_txo.clone(), ChannelMonitorUpdate {
+		(funding_txo, ChannelMonitorUpdate {
 			update_id: self.latest_monitor_update_id,
 			updates: vec![ChannelMonitorUpdateStep::ChannelForceClosed { should_broadcast }],
 		}, dropped_outbound_htlcs)

lightning/src/ln/channelmanager.rs

@@ -2363,7 +2363,12 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 					// channel, not the temporary_channel_id. This is compatible with ourselves, but the
 					// spec is somewhat ambiguous here. Not a huge deal since we'll send error messages for
 					// any messages referencing a previously-closed channel anyway.
-					return Err(MsgHandleErrInternal::from_finish_shutdown("ChannelMonitor storage failure".to_owned(), funding_msg.channel_id, chan.force_shutdown(true), None));
+					// We do not do a force-close here as that would generate a monitor update for
+					// a monitor that we didn't manage to store (and that we don't care about - we
+					// don't respond with the funding_signed so the channel can never go on chain).
+					let (_funding_txo_option, _monitor_update, failed_htlcs) = chan.force_shutdown(true);
+					assert!(failed_htlcs.is_empty());
+					return Err(MsgHandleErrInternal::send_err_msg_no_close("ChannelMonitor storage failure".to_owned(), funding_msg.channel_id));
 				},
 				ChannelMonitorUpdateErr::TemporaryFailure => {
 					// There's no problem signing a counterparty's funding transaction if our monitor

lightning/src/ln/functional_test_utils.rs

@@ -487,7 +487,11 @@ pub fn create_announced_chan_between_nodes<'a, 'b, 'c, 'd>(nodes: &'a Vec<Node<'
 
 pub fn create_announced_chan_between_nodes_with_value<'a, 'b, 'c, 'd>(nodes: &'a Vec<Node<'b, 'c, 'd>>, a: usize, b: usize, channel_value: u64, push_msat: u64, a_flags: InitFeatures, b_flags: InitFeatures) -> (msgs::ChannelUpdate, msgs::ChannelUpdate, [u8; 32], Transaction) {
 	let chan_announcement = create_chan_between_nodes_with_value(&nodes[a], &nodes[b], channel_value, push_msat, a_flags, b_flags);
+	update_nodes_with_chan_announce(nodes, a, b, &chan_announcement.0, &chan_announcement.1, &chan_announcement.2);
+	(chan_announcement.1, chan_announcement.2, chan_announcement.3, chan_announcement.4)
+}
 
+pub fn update_nodes_with_chan_announce<'a, 'b, 'c, 'd>(nodes: &'a Vec<Node<'b, 'c, 'd>>, a: usize, b: usize, ann: &msgs::ChannelAnnouncement, upd_1: &msgs::ChannelUpdate, upd_2: &msgs::ChannelUpdate) {
 	nodes[a].node.broadcast_node_announcement([0, 0, 0], [0; 32], Vec::new());
 	let a_events = nodes[a].node.get_and_clear_pending_msg_events();
 	assert_eq!(a_events.len(), 1);
@@ -509,13 +513,12 @@ pub fn create_announced_chan_between_nodes_with_value<'a, 'b, 'c, 'd>(nodes: &'a
 	};
 
 	for node in nodes {
-		assert!(node.net_graph_msg_handler.handle_channel_announcement(&chan_announcement.0).unwrap());
-		node.net_graph_msg_handler.handle_channel_update(&chan_announcement.1).unwrap();
-		node.net_graph_msg_handler.handle_channel_update(&chan_announcement.2).unwrap();
+		assert!(node.net_graph_msg_handler.handle_channel_announcement(ann).unwrap());
+		node.net_graph_msg_handler.handle_channel_update(upd_1).unwrap();
+		node.net_graph_msg_handler.handle_channel_update(upd_2).unwrap();
 		node.net_graph_msg_handler.handle_node_announcement(&a_node_announcement).unwrap();
 		node.net_graph_msg_handler.handle_node_announcement(&b_node_announcement).unwrap();
 	}
-	(chan_announcement.1, chan_announcement.2, chan_announcement.3, chan_announcement.4)
 }
 
 macro_rules! check_spends {

lightning/src/ln/functional_tests.rs

@@ -8457,6 +8457,43 @@ fn test_concurrent_monitor_claim() {
 	}
 }
 
+#[test]
+fn test_pre_lockin_no_chan_closed_update() {
+	// Test that if a peer closes a channel in response to a funding_created message we don't
+	// generate a channel update (as the channel cannot appear on chain without a funding_signed
+	// message).
+	//
+	// Doing so would imply a channel monitor update before the initial channel monitor
+	// registration, violating our API guarantees.
+	//
+	// Previously, full_stack_target managed to hit this case by opening then closing a channel,
+	// then opening a second channel with the same funding output as the first (which is not
+	// rejected because the first channel does not exist in the ChannelManager) and closing it
+	// before receiving funding_signed.
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	// Create an initial channel
+	nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42, None).unwrap();
+	let mut open_chan_msg = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
+	nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), InitFeatures::known(), &open_chan_msg);
+	let accept_chan_msg = get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id());
+	nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), InitFeatures::known(), &accept_chan_msg);
+
+	// Move the first channel through the funding flow...
+	let (temporary_channel_id, _tx, funding_output) = create_funding_transaction(&nodes[0], 100000, 42);
+
+	nodes[0].node.funding_transaction_generated(&temporary_channel_id, funding_output);
+	check_added_monitors!(nodes[0], 0);
+
+	let funding_created_msg = get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id());
+	let channel_id = ::chain::transaction::OutPoint { txid: funding_created_msg.funding_txid, index: funding_created_msg.funding_output_index }.to_channel_id();
+	nodes[0].node.handle_error(&nodes[1].node.get_our_node_id(), &msgs::ErrorMessage { channel_id, data: "Hi".to_owned() });
+	assert!(nodes[0].chain_monitor.added_monitors.lock().unwrap().is_empty());
+}
+
 #[test]
 fn test_htlc_no_detection() {
 	// This test is a mutation to underscore the detection logic bug we had
@@ -8687,3 +8724,143 @@ fn test_onchain_htlc_settlement_after_close() {
 	do_test_onchain_htlc_settlement_after_close(true, false);
 	do_test_onchain_htlc_settlement_after_close(false, false);
 }
+
+#[test]
+fn test_duplicate_chan_id() {
+	// Test that if a given peer tries to open a channel with the same channel_id as one that is
+	// already open we reject it and keep the old channel.
+	//
+	// Previously, full_stack_target managed to figure out that if you tried to open two channels
+	// with the same funding output (ie post-funding channel_id), we'd create a monitor update for
+	// the existing channel when we detect the duplicate new channel, screwing up our monitor
+	// updating logic for the existing channel.
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	// Create an initial channel
+	nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42, None).unwrap();
+	let mut open_chan_msg = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
+	nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), InitFeatures::known(), &open_chan_msg);
+	nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), InitFeatures::known(), &get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id()));
+
+	// Try to create a second channel with the same temporary_channel_id as the first and check
+	// that it is rejected.
+	nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), InitFeatures::known(), &open_chan_msg);
+	{
+		let events = nodes[1].node.get_and_clear_pending_msg_events();
+		assert_eq!(events.len(), 1);
+		match events[0] {
+			MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { ref msg }, node_id } => {
+				// Technically, at this point, nodes[1] would be justified in thinking both the
+				// first (valid) and second (invalid) channels are closed, given they both have
+				// the same non-temporary channel_id. However, currently we do not, so we just
+				// move forward with it.
+				assert_eq!(msg.channel_id, open_chan_msg.temporary_channel_id);
+				assert_eq!(node_id, nodes[0].node.get_our_node_id());
+			},
+			_ => panic!("Unexpected event"),
+		}
+	}
+
+	// Move the first channel through the funding flow...
+	let (temporary_channel_id, tx, funding_output) = create_funding_transaction(&nodes[0], 100000, 42);
+
+	nodes[0].node.funding_transaction_generated(&temporary_channel_id, funding_output);
+	check_added_monitors!(nodes[0], 0);
+
+	let mut funding_created_msg = get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id());
+	nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created_msg);
+	{
+		let mut added_monitors = nodes[1].chain_monitor.added_monitors.lock().unwrap();
+		assert_eq!(added_monitors.len(), 1);
+		assert_eq!(added_monitors[0].0, funding_output);
+		added_monitors.clear();
+	}
+	let funding_signed_msg = get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id());
+
+	let funding_outpoint = ::chain::transaction::OutPoint { txid: funding_created_msg.funding_txid, index: funding_created_msg.funding_output_index };
+	let channel_id = funding_outpoint.to_channel_id();
+
+	// Now we have the first channel past funding_created (ie it has a txid-based channel_id, not a
+	// temporary one).
+
+	// First try to open a second channel with a temporary channel id equal to the txid-based one.
+	// Technically this is allowed by the spec, but we don't support it and there's little reason
+	// to. Still, it shouldn't cause any other issues.
+	open_chan_msg.temporary_channel_id = channel_id;
+	nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), InitFeatures::known(), &open_chan_msg);
+	{
+		let events = nodes[1].node.get_and_clear_pending_msg_events();
+		assert_eq!(events.len(), 1);
+		match events[0] {
+			MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { ref msg }, node_id } => {
+				// Technically, at this point, nodes[1] would be justified in thinking both
+				// channels are closed, but currently we do not, so we just move forward with it.
+				assert_eq!(msg.channel_id, open_chan_msg.temporary_channel_id);
+				assert_eq!(node_id, nodes[0].node.get_our_node_id());
+			},
+			_ => panic!("Unexpected event"),
+		}
+	}
+
+	// Now try to create a second channel which has a duplicate funding output.
+	nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42, None).unwrap();
+	let open_chan_2_msg = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
+	nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), InitFeatures::known(), &open_chan_2_msg);
+	nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), InitFeatures::known(), &get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id()));
+	create_funding_transaction(&nodes[0], 100000, 42); // Get and check the FundingGenerationReady event
+
+	let funding_created = {
+		let mut a_channel_lock = nodes[0].node.channel_state.lock().unwrap();
+		let mut as_chan = a_channel_lock.by_id.get_mut(&open_chan_2_msg.temporary_channel_id).unwrap();
+		let logger = test_utils::TestLogger::new();
+		as_chan.get_outbound_funding_created(funding_outpoint, &&logger).unwrap()
+	};
+	check_added_monitors!(nodes[0], 0);
+	nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created);
+	// At this point we'll try to add a duplicate channel monitor, which will be rejected, but
+	// still needs to be cleared here.
+	check_added_monitors!(nodes[1], 1);
+
+	// ...still, nodes[1] will reject the duplicate channel.
+	{
+		let events = nodes[1].node.get_and_clear_pending_msg_events();
+		assert_eq!(events.len(), 1);
+		match events[0] {
+			MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { ref msg }, node_id } => {
+				// Technically, at this point, nodes[1] would be justified in thinking both
+				// channels are closed, but currently we do not, so we just move forward with it.
+				assert_eq!(msg.channel_id, channel_id);
+				assert_eq!(node_id, nodes[0].node.get_our_node_id());
+			},
+			_ => panic!("Unexpected event"),
+		}
+	}
+
+	// finally, finish creating the original channel and send a payment over it to make sure
+	// everything is functional.
+	nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &funding_signed_msg);
+	{
+		let mut added_monitors = nodes[0].chain_monitor.added_monitors.lock().unwrap();
+		assert_eq!(added_monitors.len(), 1);
+		assert_eq!(added_monitors[0].0, funding_output);
+		added_monitors.clear();
+	}
+
+	let events_4 = nodes[0].node.get_and_clear_pending_events();
+	assert_eq!(events_4.len(), 1);
+	match events_4[0] {
+		Event::FundingBroadcastSafe { ref funding_txo, user_channel_id } => {
+			assert_eq!(user_channel_id, 42);
+			assert_eq!(*funding_txo, funding_output);
+		},
+		_ => panic!("Unexpected event"),
+	};
+
+	let (funding_locked, _) = create_chan_between_nodes_with_value_confirm(&nodes[0], &nodes[1], &tx);
+	let (announcement, as_update, bs_update) = create_chan_between_nodes_with_value_b(&nodes[0], &nodes[1], &funding_locked);
+	update_nodes_with_chan_announce(&nodes, 0, 1, &announcement, &as_update, &bs_update);
+	send_payment(&nodes[0], &[&nodes[1]], 8000000, 8_000_000);
+}