-f address Matt/Val's comments

Author: Antoine Riard

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -258,7 +258,7 @@ impl MsgHandleErrInternal {
 					},
 				},
 			},
-			chan_id: Some(channel_id),
+			chan_id: None,
 			shutdown_finish: None,
 		}
 	}
@@ -837,7 +837,7 @@ macro_rules! handle_error {
 						});
 					}
 					if let Some(channel_id) = chan_id {
-						$self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id,  err: ClosureReason::ProcessingError { err: err.err.clone() } });
+						$self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id,  reason: ClosureReason::ProcessingError { err: err.err.clone() } });
 					}
 				}
 
@@ -1447,6 +1447,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		}
 	}
 
+	/// `peer_node_id` should be set when we receive a message from a peer, but not set when the user closes, which will be re-exposed as the `ChannelClosed`
+	/// reason.
 	fn force_close_channel_with_peer(&self, channel_id: &[u8; 32], peer_node_id: Option<&PublicKey>, peer_msg: Option<&String>) -> Result<PublicKey, APIError> {
 		let mut chan = {
 			let mut channel_state_lock = self.channel_state.lock().unwrap();
@@ -1463,12 +1465,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 				let mut pending_events_lock = self.pending_events.lock().unwrap();
 				if peer_node_id.is_some() {
 					if let Some(peer_msg) = peer_msg {
-						pending_events_lock.push(events::Event::ChannelClosed { channel_id: *channel_id, err: ClosureReason::CounterpartyForceClosed { peer_msg: Some(peer_msg.to_string()) } });
-					} else {
-						pending_events_lock.push(events::Event::ChannelClosed { channel_id: *channel_id, err: ClosureReason::CounterpartyForceClosed { peer_msg: None } });
+						pending_events_lock.push(events::Event::ChannelClosed { channel_id: *channel_id, reason: ClosureReason::CounterpartyForceClosed { peer_msg: peer_msg.to_string() } });
 					}
 				} else {
-					pending_events_lock.push(events::Event::ChannelClosed { channel_id: *channel_id, err: ClosureReason::HolderForceClosed });
+					pending_events_lock.push(events::Event::ChannelClosed { channel_id: *channel_id, reason: ClosureReason::HolderForceClosed });
 				}
 				chan.remove_entry().1
 			} else {
@@ -2435,7 +2435,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 											if let Some(short_id) = channel.get_short_channel_id() {
 												channel_state.short_to_id.remove(&short_id);
 											}
-											// ChannelClosed event is generated by handle_errors for us.
+											// ChannelClosed event is generated by handle_error for us.
 											Err(MsgHandleErrInternal::from_finish_shutdown(msg, channel_id, channel.force_shutdown(true), self.get_channel_update_for_broadcast(&channel).ok()))
 										},
 										ChannelError::CloseDelayBroadcast(_) => { panic!("Wait is only generated on receipt of channel_reestablish, which is handled by try_chan_entry, we don't bother to support it here"); }
@@ -3565,8 +3565,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 					msg: update
 				});
 			}
-			//TODO: split between CounterpartyInitiated/LocallyInitiated
-			self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: msg.channel_id,  err: ClosureReason::CooperativeClosure });
+			self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: msg.channel_id,  reason: ClosureReason::CooperativeClosure });
 		}
 		Ok(())
 	}
@@ -3978,7 +3977,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 								msg: update
 							});
 						}
-						self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: chan.channel_id(),  err: ClosureReason::CommitmentTxBroadcasted });
+						self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: chan.channel_id(),  reason: ClosureReason::CommitmentTxBroadcasted });
 						pending_msg_events.push(events::MessageSendEvent::HandleError {
 							node_id: chan.get_counterparty_node_id(),
 							action: msgs::ErrorAction::SendErrorMessage {
@@ -4514,7 +4513,7 @@ where
 							msg: update
 						});
 					}
-					self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: channel.channel_id(),  err: ClosureReason::CommitmentTxBroadcasted });
+					self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: channel.channel_id(),  reason: ClosureReason::CommitmentTxBroadcasted });
 					pending_msg_events.push(events::MessageSendEvent::HandleError {
 						node_id: channel.get_counterparty_node_id(),
 						action: msgs::ErrorAction::SendErrorMessage { msg: e },
@@ -4705,7 +4704,7 @@ impl<Signer: Sign, M: Deref , T: Deref , K: Deref , F: Deref , L: Deref >
 								msg: update
 							});
 						}
-						self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: chan.channel_id(),  err: ClosureReason::DisconnectedPeer });
+						self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: chan.channel_id(),  reason: ClosureReason::DisconnectedPeer });
 						false
 					} else {
 						true
@@ -4720,7 +4719,7 @@ impl<Signer: Sign, M: Deref , T: Deref , K: Deref , F: Deref , L: Deref >
 							if let Some(short_id) = chan.get_short_channel_id() {
 								short_to_id.remove(&short_id);
 							}
-							self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: chan.channel_id(),  err: ClosureReason::DisconnectedPeer });
+							self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: chan.channel_id(),  reason: ClosureReason::DisconnectedPeer });
 							return false;
 						} else {
 							no_channels_remain = false;
@@ -5679,10 +5678,8 @@ mod tests {
 		nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
 		check_added_monitors!(nodes[1], 0);
 		commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false);
-		let events = nodes[1].node.get_and_clear_pending_events();
-		expect_pending_htlcs_forwardable!(nodes[1], events);
-		let events = nodes[1].node.get_and_clear_pending_events();
-		expect_pending_htlcs_forwardable!(nodes[1], events);
+		expect_pending_htlcs_forwardable!(nodes[1]);
+		expect_pending_htlcs_forwardable!(nodes[1]);
 		check_added_monitors!(nodes[1], 1);
 		let updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
 		assert!(updates.update_add_htlcs.is_empty());
@@ -5747,7 +5744,7 @@ mod tests {
 
 	#[test]
 	fn test_keysend_dup_payment_hash() {
-		return true;
+		return;
 
 		// (1): Test that a keysend payment with a duplicate payment hash to an existing pending
 		//      outbound regular payment fails as expected.
@@ -5816,10 +5813,8 @@ mod tests {
 		nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
 		check_added_monitors!(nodes[1], 0);
 		commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false);
-		let events = nodes[1].node.get_and_clear_pending_events();
-		expect_pending_htlcs_forwardable!(nodes[1], events);
-		let events = nodes[1].node.get_and_clear_pending_events();
-		expect_pending_htlcs_forwardable!(nodes[1], events);
+		expect_pending_htlcs_forwardable!(nodes[1]);
+		expect_pending_htlcs_forwardable!(nodes[1]);
 		check_added_monitors!(nodes[1], 1);
 		let updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
 		assert!(updates.update_add_htlcs.is_empty());

lightning/src/ln/channelmanager.rs

@@ -770,7 +770,9 @@ macro_rules! check_closed_event {
 		let events = $node.node.get_and_clear_pending_events();
 		assert_eq!(events.len(), $events);
 		match events[0] {
-			Event::ChannelClosed { .. } => {}
+			Event::ChannelClosed { ref reason, .. } => {
+				//assert_eq!(reason, $reason);
+			},
 			_ => panic!("Unexpected event"),
 		}
 	}}
@@ -940,8 +942,7 @@ macro_rules! commitment_signed_dance {
 		{
 			commitment_signed_dance!($node_a, $node_b, $commitment_signed, $fail_backwards, true);
 			if $fail_backwards {
-				let events = $node_a.node.get_and_clear_pending_events();
-				expect_pending_htlcs_forwardable!($node_a, events);
+				expect_pending_htlcs_forwardable!($node_a);
 				check_added_monitors!($node_a, 1);
 
 				let channel_state = $node_a.node.channel_state.lock().unwrap();
@@ -983,22 +984,37 @@ macro_rules! get_route_and_payment_hash {
 }
 
 macro_rules! expect_pending_htlcs_forwardable_ignore {
-	($node: expr, $events: expr) => {{
-		assert_eq!($events.len(), 1);
-		match $events[0] {
+	($node: expr) => {{
+		let events = $node.node.get_and_clear_pending_events();
+		assert_eq!(events.len(), 1);
+		match events[0] {
 			Event::PendingHTLCsForwardable { .. } => { },
 			_ => panic!("Unexpected event"),
 		};
 	}}
 }
 
 macro_rules! expect_pending_htlcs_forwardable {
-	($node: expr, $events: expr) => {{
-		expect_pending_htlcs_forwardable_ignore!($node, $events);
+	($node: expr) => {{
+		expect_pending_htlcs_forwardable_ignore!($node);
 		$node.node.process_pending_htlc_forwards();
 	}}
 }
 
+#[cfg(test)]
+macro_rules! expect_pending_htlcs_forwardable_from_events {
+	($node: expr, $events: expr, $ignore: expr) => {{
+		assert_eq!($events.len(), 1);
+		match $events[0] {
+			Event::PendingHTLCsForwardable { .. } => { },
+			_ => panic!("Unexpected event"),
+		};
+		if $ignore {
+			$node.node.process_pending_htlc_forwards();
+		}
+	}}
+}
+
 #[cfg(any(test, feature = "unstable"))]
 macro_rules! expect_payment_received {
 	($node: expr, $expected_payment_hash: expr, $expected_payment_secret: expr, $expected_recv_value: expr) => {
@@ -1114,8 +1130,7 @@ pub fn pass_along_path<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_path
 		check_added_monitors!(node, 0);
 		commitment_signed_dance!(node, prev_node, payment_event.commitment_msg, false);
 
-		let events = node.node.get_and_clear_pending_events();
-		expect_pending_htlcs_forwardable!(node, events);
+		expect_pending_htlcs_forwardable!(node);
 
 		if idx == expected_path.len() - 1 {
 			let events_2 = node.node.get_and_clear_pending_events();