From 920d1458c40f9d57679a17090c9fc090b1001d7b Mon Sep 17 00:00:00 2001 From: Yuntai Kyong Date: Mon, 22 Oct 2018 11:12:44 -0400 Subject: [PATCH] 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). --- src/ln/channelmanager.rs | 44 +++++++++++++++++++++------------------- src/ln/msgs.rs | 2 +- src/ln/peer_handler.rs | 9 ++++---- src/util/events.rs | 12 +++++++++++ src/util/test_utils.rs | 2 +- 5 files changed, 42 insertions(+), 27 deletions(-) diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index 7389a678b..1e396612c 100644 --- a/src/ln/channelmanager.rs +++ b/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::PaymentFailureNetworkUpdate { + 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, 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, 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"), } diff --git a/src/ln/msgs.rs b/src/ln/msgs.rs index 610158503..7b0254bdd 100644 --- a/src/ln/msgs.rs +++ b/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, 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. diff --git a/src/ln/peer_handler.rs b/src/ln/peer_handler.rs index 76cc35f64..81ef41cf7 100644 --- a/src/ln/peer_handler.rs +++ b/src/ln/peer_handler.rs @@ -615,10 +615,7 @@ impl PeerManager { }, 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 PeerManager { } continue; }, + Event::PaymentFailureNetworkUpdate { 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 { diff --git a/src/util/events.rs b/src/util/events.rs index a317f0766..51417c63c 100644 --- a/src/util/events.rs +++ b/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,14 @@ pub enum Event { node_id: PublicKey, /// The action which should be taken. action: Option + }, + /// When a payment fails we may receive updates back from the hop where it failed. In such + /// cases this event is generated so that we can inform the router of this information. + /// + /// This event is handled by PeerManager::process_events if you are using a PeerManager. + PaymentFailureNetworkUpdate { + /// The channel/node update which should be sent to router + update: msgs::HTLCFailChannelUpdate, } } diff --git a/src/util/test_utils.rs b/src/util/test_utils.rs index 49aa269b3..f8341e885 100644 --- a/src/util/test_utils.rs +++ b/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, 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> { -- 2.39.5