Check the PK of the source of an error before closing chans from it

lightning/src/ln/channelmanager.rs

@@ -916,19 +916,22 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 		}
 	}
 
-	/// Force closes a channel, immediately broadcasting the latest local commitment transaction to
-	/// the chain and rejecting new HTLCs on the given channel. Fails if channel_id is unknown to the manager.
-	pub fn force_close_channel(&self, channel_id: &[u8; 32]) -> Result<(), APIError>{
-		let _consistency_lock = self.total_consistency_lock.read().unwrap();
-
+	fn force_close_channel_with_peer(&self, channel_id: &[u8; 32], peer_node_id: Option<&PublicKey>) -> Result<(), APIError> {
 		let mut chan = {
 			let mut channel_state_lock = self.channel_state.lock().unwrap();
 			let channel_state = &mut *channel_state_lock;
-			if let Some(chan) = channel_state.by_id.remove(channel_id) {
-				if let Some(short_id) = chan.get_short_channel_id() {
+			if let hash_map::Entry::Occupied(chan) = channel_state.by_id.entry(channel_id.clone()) {
+				if let Some(node_id) = peer_node_id {
+					if chan.get().get_counterparty_node_id() != *node_id {
+						// Error or Ok here doesn't matter - the result is only exposed publicly
+						// when peer_node_id is None anyway.
+						return Ok(());
+					}
+				}
+				if let Some(short_id) = chan.get().get_short_channel_id() {
 					channel_state.short_to_id.remove(&short_id);
 				}
-				chan
+				chan.remove_entry().1
 			} else {
 				return Err(APIError::ChannelUnavailable{err: "No such channel".to_owned()});
 			}
@@ -945,6 +948,13 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 		Ok(())
 	}
 
+	/// Force closes a channel, immediately broadcasting the latest local commitment transaction to
+	/// the chain and rejecting new HTLCs on the given channel. Fails if channel_id is unknown to the manager.
+	pub fn force_close_channel(&self, channel_id: &[u8; 32]) -> Result<(), APIError> {
+		let _consistency_lock = self.total_consistency_lock.read().unwrap();
+		self.force_close_channel_with_peer(channel_id, None)
+	}
+
 	/// Force close all channels, immediately broadcasting the latest local commitment transaction
 	/// for each to the chain and rejecting new HTLCs on each.
 	pub fn force_close_all_channels(&self) {
@@ -3474,12 +3484,12 @@ impl<ChanSigner: ChannelKeys, M: Deref + Sync + Send, T: Deref + Sync + Send, K:
 			for chan in self.list_channels() {
 				if chan.remote_network_id == *counterparty_node_id {
 					// Untrusted messages from peer, we throw away the error if id points to a non-existent channel
-					let _ = self.force_close_channel(&msg.channel_id);
+					let _ = self.force_close_channel_with_peer(&chan.channel_id, Some(counterparty_node_id));
 				}
 			}
 		} else {
 			// Untrusted messages from peer, we throw away the error if id points to a non-existent channel
-			let _ = self.force_close_channel(&msg.channel_id);
+			let _ = self.force_close_channel_with_peer(&msg.channel_id, Some(counterparty_node_id));
 		}
 	}
 }

lightning/src/ln/functional_tests.rs

@@ -8836,3 +8836,66 @@ fn test_duplicate_chan_id() {
 	update_nodes_with_chan_announce(&nodes, 0, 1, &announcement, &as_update, &bs_update);
 	send_payment(&nodes[0], &[&nodes[1]], 8000000, 8_000_000);
 }
+
+#[test]
+fn test_error_chans_closed() {
+	// Test that we properly handle error messages, closing appropriate channels.
+	//
+	// Prior to #787 we'd allow a peer to make us force-close a channel we had with a different
+	// peer. The "real" fix for that is to index channels with peers_ids, however in the mean time
+	// we can test various edge cases around it to ensure we don't regress.
+	let chanmon_cfgs = create_chanmon_cfgs(3);
+	let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
+	let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
+
+	// Create some initial channels
+	let chan_1 = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 10001, InitFeatures::known(), InitFeatures::known());
+	let chan_2 = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 10001, InitFeatures::known(), InitFeatures::known());
+	let chan_3 = create_announced_chan_between_nodes_with_value(&nodes, 0, 2, 100000, 10001, InitFeatures::known(), InitFeatures::known());
+
+	assert_eq!(nodes[0].node.list_usable_channels().len(), 3);
+	assert_eq!(nodes[1].node.list_usable_channels().len(), 2);
+	assert_eq!(nodes[2].node.list_usable_channels().len(), 1);
+
+	// Closing a channel from a different peer has no effect
+	nodes[0].node.handle_error(&nodes[1].node.get_our_node_id(), &msgs::ErrorMessage { channel_id: chan_3.2, data: "ERR".to_owned() });
+	assert_eq!(nodes[0].node.list_usable_channels().len(), 3);
+
+	// Closing one channel doesn't impact others
+	nodes[0].node.handle_error(&nodes[1].node.get_our_node_id(), &msgs::ErrorMessage { channel_id: chan_2.2, data: "ERR".to_owned() });
+	check_added_monitors!(nodes[0], 1);
+	check_closed_broadcast!(nodes[0], false);
+	assert_eq!(nodes[0].node.list_usable_channels().len(), 2);
+	assert!(nodes[0].node.list_usable_channels()[0].channel_id == chan_1.2 || nodes[0].node.list_usable_channels()[1].channel_id == chan_1.2);
+	assert!(nodes[0].node.list_usable_channels()[0].channel_id == chan_3.2 || nodes[0].node.list_usable_channels()[1].channel_id == chan_3.2);
+
+	// A null channel ID should close all channels
+	let _chan_4 = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 10001, InitFeatures::known(), InitFeatures::known());
+	nodes[0].node.handle_error(&nodes[1].node.get_our_node_id(), &msgs::ErrorMessage { channel_id: [0; 32], data: "ERR".to_owned() });
+	check_added_monitors!(nodes[0], 2);
+	let events = nodes[0].node.get_and_clear_pending_msg_events();
+	assert_eq!(events.len(), 2);
+	match events[0] {
+		MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
+			assert_eq!(msg.contents.flags & 2, 2);
+		},
+		_ => panic!("Unexpected event"),
+	}
+	match events[1] {
+		MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
+			assert_eq!(msg.contents.flags & 2, 2);
+		},
+		_ => panic!("Unexpected event"),
+	}
+	// Note that at this point users of a standard PeerHandler will end up calling
+	// peer_disconnected with no_connection_possible set to false, duplicating the
+	// close-all-channels logic. That's OK, we don't want to end up not force-closing channels for
+	// users with their own peer handling logic. We duplicate the call here, however.
+	assert_eq!(nodes[0].node.list_usable_channels().len(), 1);
+	assert!(nodes[0].node.list_usable_channels()[0].channel_id == chan_3.2);
+
+	nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), true);
+	assert_eq!(nodes[0].node.list_usable_channels().len(), 1);
+	assert!(nodes[0].node.list_usable_channels()[0].channel_id == chan_3.2);
+}

lightning/src/ln/peer_handler.rs

@@ -88,10 +88,8 @@ pub trait SocketDescriptor : cmp::Eq + hash::Hash + Clone {
 }
 
 /// Error for PeerManager errors. If you get one of these, you must disconnect the socket and
-/// generate no further read_event/write_buffer_space_avail calls for the descriptor, only
-/// triggering a single socket_disconnected call (unless it was provided in response to a
-/// new_*_connection event, in which case no such socket_disconnected() must be called and the
-/// socket silently disconencted).
+/// generate no further read_event/write_buffer_space_avail/socket_disconnected calls for the
+/// descriptor.
 pub struct PeerHandleError {
 	/// Used to indicate that we probably can't make any future connections to this peer, implying
 	/// we should go ahead and force-close any channels we have with it.