More regularly send an Error message when we force-close a channel

When we force-close a channel, for whatever reason, it is nice to
send an error message to our peer. This allows them to closes the
channel on their end instead of trying to send through it and
failing. Further, it may induce them to broadcast their commitment
transaction, possibly getting that confirmed and saving us on fees.

This commit adds a few more cases where we should have been sending
error messages but weren't. It also includes an almost-global
replace in tests of the second argument in
`check_closed_broadcast!()` from false to true (indicating an error
message is expected). There are only a few exceptions, notably
those where the closure is the result of our counterparty having
sent *us* an error message.

Author: TheBlueMatt

lightning-persister/src/lib.rs

@@ -241,15 +241,15 @@ mod tests {
 		// Force close because cooperative close doesn't result in any persisted
 		// updates.
 		nodes[0].node.force_close_channel(&nodes[0].node.list_channels()[0].channel_id).unwrap();
-		check_closed_broadcast!(nodes[0], false);
+		check_closed_broadcast!(nodes[0], true);
 		check_added_monitors!(nodes[0], 1);
 
 		let node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
 		assert_eq!(node_txn.len(), 1);
 
 		let header = BlockHeader { version: 0x20000000, prev_blockhash: nodes[0].best_block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
 		connect_block(&nodes[1], &Block { header, txdata: vec![node_txn[0].clone(), node_txn[0].clone()]});
-		check_closed_broadcast!(nodes[1], false);
+		check_closed_broadcast!(nodes[1], true);
 		check_added_monitors!(nodes[1], 1);
 
 		// Make sure everything is persisted as expected after close.

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -241,7 +241,7 @@ fn do_test_simple_monitor_temporary_update_fail(disconnect: bool, persister_fail
 	// ...and make sure we can force-close a frozen channel
 	nodes[0].node.force_close_channel(&channel_id).unwrap();
 	check_added_monitors!(nodes[0], 1);
-	check_closed_broadcast!(nodes[0], false);
+	check_closed_broadcast!(nodes[0], true);
 
 	// TODO: Once we hit the chain with the failure transaction we should check that we get a
 	// PaymentFailed event

lightning/src/ln/channelmanager.rs

@@ -1002,16 +1002,14 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		}
 	}
 
-	fn force_close_channel_with_peer(&self, channel_id: &[u8; 32], peer_node_id: Option<&PublicKey>) -> Result<(), APIError> {
+	fn force_close_channel_with_peer(&self, channel_id: &[u8; 32], peer_node_id: Option<&PublicKey>) -> Result<PublicKey, APIError> {
 		let mut chan = {
 			let mut channel_state_lock = self.channel_state.lock().unwrap();
 			let channel_state = &mut *channel_state_lock;
 			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(());
+						return Err(APIError::ChannelUnavailable{err: "No such channel".to_owned()});
 					}
 				}
 				if let Some(short_id) = chan.get().get_short_channel_id() {
@@ -1031,14 +1029,27 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 			});
 		}
 
-		Ok(())
+		Ok(chan.get_counterparty_node_id())
 	}
 
 	/// 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 _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
-		self.force_close_channel_with_peer(channel_id, None)
+		match self.force_close_channel_with_peer(channel_id, None) {
+			Ok(pk) => {
+				self.channel_state.lock().unwrap().pending_msg_events.push(
+					events::MessageSendEvent::HandleError {
+						node_id: pk,
+						action: msgs::ErrorAction::SendErrorMessage {
+							msg: msgs::ErrorMessage { channel_id: *channel_id, data: "Channel force-closed".to_owned() }
+						},
+					}
+				);
+				Ok(())
+			},
+			Err(e) => Err(e)
+		}
 	}
 
 	/// Force close all channels, immediately broadcasting the latest local commitment transaction
@@ -3194,6 +3205,12 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 									msg: update
 								});
 							}
+							pending_msg_events.push(events::MessageSendEvent::HandleError {
+								node_id: chan.get_counterparty_node_id(),
+								action: msgs::ErrorAction::SendErrorMessage {
+									msg: msgs::ErrorMessage { channel_id: chan.channel_id(), data: "Channel force-closed".to_owned() }
+								},
+							});
 						}
 					},
 				}
@@ -3364,6 +3381,12 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 										msg: update
 									});
 								}
+								pending_msg_events.push(events::MessageSendEvent::HandleError {
+									node_id: channel.get_counterparty_node_id(),
+									action: msgs::ErrorAction::SendErrorMessage {
+										msg: msgs::ErrorMessage { channel_id: channel.channel_id(), data: "Commitment transaction was confirmed on chain.".to_owned() }
+									},
+								});
 								return false;
 							}
 						}
@@ -3433,7 +3456,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 			let channel_state = &mut *channel_lock;
 			let short_to_id = &mut channel_state.short_to_id;
 			let pending_msg_events = &mut channel_state.pending_msg_events;
-			channel_state.by_id.retain(|_,  v| {
+			channel_state.by_id.retain(|channel_id,  v| {
 				if v.block_disconnected(header, new_height) {
 					if let Some(short_id) = v.get_short_channel_id() {
 						short_to_id.remove(&short_id);
@@ -3444,6 +3467,12 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 							msg: update
 						});
 					}
+					pending_msg_events.push(events::MessageSendEvent::HandleError {
+						node_id: v.get_counterparty_node_id(),
+						action: msgs::ErrorAction::SendErrorMessage {
+							msg: msgs::ErrorMessage { channel_id: *channel_id, data: "Funding transaction was un-confirmed.".to_owned() }
+						},
+					});
 					false
 				} else {
 					true

lightning/src/ln/functional_test_utils.rs

@@ -1345,22 +1345,36 @@ pub fn check_preimage_claim<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, prev_txn: &Vec<
 
 pub fn get_announce_close_broadcast_events<'a, 'b, 'c>(nodes: &Vec<Node<'a, 'b, 'c>>, a: usize, b: usize)  {
 	let events_1 = nodes[a].node.get_and_clear_pending_msg_events();
-	assert_eq!(events_1.len(), 1);
+	assert_eq!(events_1.len(), 2);
 	let as_update = match events_1[0] {
 		MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
 			msg.clone()
 		},
 		_ => panic!("Unexpected event"),
 	};
+	match events_1[1] {
+		MessageSendEvent::HandleError { node_id, action: msgs::ErrorAction::SendErrorMessage { ref msg } } => {
+			assert_eq!(node_id, nodes[b].node.get_our_node_id());
+			assert_eq!(msg.data, "Commitment transaction was confirmed on chain.");
+		},
+		_ => panic!("Unexpected event"),
+	}
 
 	let events_2 = nodes[b].node.get_and_clear_pending_msg_events();
-	assert_eq!(events_2.len(), 1);
+	assert_eq!(events_2.len(), 2);
 	let bs_update = match events_2[0] {
 		MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
 			msg.clone()
 		},
 		_ => panic!("Unexpected event"),
 	};
+	match events_2[1] {
+		MessageSendEvent::HandleError { node_id, action: msgs::ErrorAction::SendErrorMessage { ref msg } } => {
+			assert_eq!(node_id, nodes[a].node.get_our_node_id());
+			assert_eq!(msg.data, "Commitment transaction was confirmed on chain.");
+		},
+		_ => panic!("Unexpected event"),
+	}
 
 	for node in nodes {
 		node.net_graph_msg_handler.handle_channel_update(&as_update).unwrap();