Move HTLCFailChannelUpdate handling out-of-band

While this isn't neccessary for message ordering consistency, this
does mean that we won't end up processing an HTLCFailChannelUpdate
from a update_fail_htlc prior to it being fully committed (where if
the peer disconnects/reconnects it could theoretically give us a
different result, eg if their next-hop reconnected to them).

Author: yuntai

src/ln/channelmanager.rs

@@ -1381,11 +1381,21 @@ impl ChannelManager {
 		match source {
 			HTLCSource::OutboundRoute { .. } => {
 				mem::drop(channel_state);
-
-				let mut pending_events = self.pending_events.lock().unwrap();
-				pending_events.push(events::Event::PaymentFailed {
-					payment_hash: payment_hash.clone()
-				});
+				if let &HTLCFailReason::ErrorPacket { ref err } = &onion_error {
+					let (channel_update, payment_retryable) = self.process_onion_failure(&source, err.data.clone());
+					let mut pending_events = self.pending_events.lock().unwrap();
+					if let Some(channel_update) = channel_update {
+						pending_events.push(events::Event::RouteUpdate {
+							update: channel_update,
+						});
+					}
+					pending_events.push(events::Event::PaymentFailed {
+						payment_hash: payment_hash.clone(),
+						rejected_by_dest: !payment_retryable,
+					});
+				} else {
+					panic!("should have onion error packet here");
+				}
 			},
 			HTLCSource::PreviousHopData(HTLCPreviousHopData { short_channel_id, htlc_id, incoming_packet_shared_secret }) => {
 				let err_packet = match onion_error {
@@ -1996,9 +2006,9 @@ impl ChannelManager {
 		} else { ((None, true)) }
 	}
 
-	fn internal_update_fail_htlc(&self, their_node_id: &PublicKey, msg: &msgs::UpdateFailHTLC) -> Result<Option<msgs::HTLCFailChannelUpdate>, MsgHandleErrInternal> {
+	fn internal_update_fail_htlc(&self, their_node_id: &PublicKey, msg: &msgs::UpdateFailHTLC) -> Result<(), MsgHandleErrInternal> {
 		let mut channel_state = self.channel_state.lock().unwrap();
-		let htlc_source = match channel_state.by_id.get_mut(&msg.channel_id) {
+		match channel_state.by_id.get_mut(&msg.channel_id) {
 			Some(chan) => {
 				if chan.get_their_node_id() != *their_node_id {
 					//TODO: here and below MsgHandleErrInternal, #153 case
@@ -2009,17 +2019,7 @@ impl ChannelManager {
 			},
 			None => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel", msg.channel_id))
 		}?;
-
-		// we are the origin node and update route information
-		// also determine if the payment is retryable
-		if let &HTLCSource::OutboundRoute { .. } = htlc_source {
-			let (channel_update, _payment_retry) = self.process_onion_failure(htlc_source, msg.reason.data.clone());
-			Ok(channel_update)
-			// TODO: include pyament_retry info in PaymentFailed event that will be
-			// fired when receiving revoke_and_ack
-		} else {
-			Ok(None)
-		}
+		Ok(())
 	}
 
 	fn internal_update_fail_malformed_htlc(&self, their_node_id: &PublicKey, msg: &msgs::UpdateFailMalformedHTLC) -> Result<(), MsgHandleErrInternal> {
@@ -2424,7 +2424,7 @@ impl ChannelMessageHandler for ChannelManager {
 		handle_error!(self, self.internal_update_fulfill_htlc(their_node_id, msg), their_node_id)
 	}
 
-	fn handle_update_fail_htlc(&self, their_node_id: &PublicKey, msg: &msgs::UpdateFailHTLC) -> Result<Option<msgs::HTLCFailChannelUpdate>, HandleError> {
+	fn handle_update_fail_htlc(&self, their_node_id: &PublicKey, msg: &msgs::UpdateFailHTLC) -> Result<(), HandleError> {
 		handle_error!(self, self.internal_update_fail_htlc(their_node_id, msg), their_node_id)
 	}
 
@@ -3296,8 +3296,9 @@ mod tests {
 			let events = origin_node.node.get_and_clear_pending_events();
 			assert_eq!(events.len(), 1);
 			match events[0] {
-				Event::PaymentFailed { payment_hash } => {
+				Event::PaymentFailed { payment_hash, rejected_by_dest } => {
 					assert_eq!(payment_hash, our_payment_hash);
+					assert!(rejected_by_dest);
 				},
 				_ => panic!("Unexpected event"),
 			}
@@ -5064,8 +5065,9 @@ mod tests {
 				_ => panic!("Unexpected event"),
 			}
 			match events[1] {
-				Event::PaymentFailed { payment_hash } => {
+				Event::PaymentFailed { payment_hash, rejected_by_dest } => {
 					assert_eq!(payment_hash, payment_hash_5);
+					assert!(rejected_by_dest);
 				},
 				_ => panic!("Unexpected event"),
 			}

src/ln/msgs.rs

@@ -529,7 +529,7 @@ pub trait ChannelMessageHandler : events::EventsProvider + Send + Sync {
 	/// Handle an incoming update_fulfill_htlc message from the given peer.
 	fn handle_update_fulfill_htlc(&self, their_node_id: &PublicKey, msg: &UpdateFulfillHTLC) -> Result<(), HandleError>;
 	/// Handle an incoming update_fail_htlc message from the given peer.
-	fn handle_update_fail_htlc(&self, their_node_id: &PublicKey, msg: &UpdateFailHTLC) -> Result<Option<HTLCFailChannelUpdate>, HandleError>;
+	fn handle_update_fail_htlc(&self, their_node_id: &PublicKey, msg: &UpdateFailHTLC) -> Result<(), HandleError>;
 	/// Handle an incoming update_fail_malformed_htlc message from the given peer.
 	fn handle_update_fail_malformed_htlc(&self, their_node_id: &PublicKey, msg: &UpdateFailMalformedHTLC) -> Result<(), HandleError>;
 	/// Handle an incoming commitment_signed message from the given peer.

src/ln/peer_handler.rs

@@ -615,10 +615,7 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
 											},
 											131 => {
 												let msg = try_potential_decodeerror!(msgs::UpdateFailHTLC::read(&mut reader));
-												let chan_update = try_potential_handleerror!(self.message_handler.chan_handler.handle_update_fail_htlc(&peer.their_node_id.unwrap(), &msg));
-												if let Some(update) = chan_update {
-													self.message_handler.route_handler.handle_htlc_fail_channel_update(&update);
-												}
+												try_potential_handleerror!(self.message_handler.chan_handler.handle_update_fail_htlc(&peer.their_node_id.unwrap(), &msg));
 											},
 											135 => {
 												let msg = try_potential_decodeerror!(msgs::UpdateFailMalformedHTLC::read(&mut reader));
@@ -906,6 +903,10 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
 						}
 						continue;
 					},
+					Event::RouteUpdate { ref update } => {
+						self.message_handler.route_handler.handle_htlc_fail_channel_update(update);
+						continue;
+					},
 					Event::HandleError { ref node_id, ref action } => {
 						if let Some(ref action) = *action {
 							match *action {

src/util/events.rs

@@ -75,6 +75,10 @@ pub enum Event {
 	PaymentFailed {
 		/// The hash which was given to ChannelManager::send_payment.
 		payment_hash: [u8; 32],
+		/// Indicates the payment was rejected for some reason by the recipient. This implies that
+		/// the payment has failed, not just the route in question. If this is not set, you may
+		/// retry the payment via a different route.
+		rejected_by_dest: bool,
 	},
 	/// Used to indicate that ChannelManager::process_pending_htlc_forwards should be called at a
 	/// time in the future.
@@ -161,6 +165,12 @@ pub enum Event {
 		node_id: PublicKey,
 		/// The action which should be taken.
 		action: Option<msgs::ErrorAction>
+	},
+	/// Used to indicate a change should be made into network map
+	///
+	RouteUpdate {
+		/// The channel/node update which should be sent to router
+		update: msgs::HTLCFailChannelUpdate,
 	}
 }
 

src/util/test_utils.rs

@@ -110,7 +110,7 @@ impl msgs::ChannelMessageHandler for TestChannelMessageHandler {
 	fn handle_update_fulfill_htlc(&self, _their_node_id: &PublicKey, _msg: &msgs::UpdateFulfillHTLC) -> Result<(), HandleError> {
 		Err(HandleError { err: "", action: None })
 	}
-	fn handle_update_fail_htlc(&self, _their_node_id: &PublicKey, _msg: &msgs::UpdateFailHTLC) -> Result<Option<msgs::HTLCFailChannelUpdate>, HandleError> {
+	fn handle_update_fail_htlc(&self, _their_node_id: &PublicKey, _msg: &msgs::UpdateFailHTLC) -> Result<(), HandleError> {
 		Err(HandleError { err: "", action: None })
 	}
 	fn handle_update_fail_malformed_htlc(&self, _their_node_id: &PublicKey, _msg: &msgs::UpdateFailMalformedHTLC) -> Result<(), HandleError> {