Swap handle_monitor_update_fail for a macro ala try_chan_entry
authorMatt Corallo <git@bluematt.me>
Mon, 26 Nov 2018 21:40:15 +0000 (16:40 -0500)
committerMatt Corallo <git@bluematt.me>
Tue, 11 Dec 2018 18:10:02 +0000 (13:10 -0500)
This resolves an API bug where send_payment may return a
MonitorUpdateFailed Err both when the payment will not be sent and
when the HTLC will be retried automatically when monitor updating
is restored. This makes it impossible for a client to know when
they should retry a payment and when they should not.

src/ln/channelmanager.rs

index a47236469719e181b9a33fce00e049502a858ae0..f6ee5f8cd9ef6d0b53972143715797d5e346a113 100644 (file)
@@ -442,6 +442,24 @@ macro_rules! try_chan_entry {
        }
 }
 
+// Does not break in case of TemporaryFailure!
+macro_rules! maybe_break_monitor_err {
+       ($self: expr, $err: expr, $channel_state: expr, $entry: expr, $action_type: path) => {
+               match $err {
+                       ChannelMonitorUpdateErr::PermanentFailure => {
+                               let (channel_id, mut chan) = $entry.remove_entry();
+                               if let Some(short_id) = chan.get_short_channel_id() {
+                                       $channel_state.short_to_id.remove(&short_id);
+                               }
+                               break Err(MsgHandleErrInternal::from_finish_shutdown("ChannelMonitor storage failure", channel_id, chan.force_shutdown(), $self.get_channel_update(&chan).ok()))
+                       },
+                       ChannelMonitorUpdateErr::TemporaryFailure => {
+                               $entry.get_mut().monitor_update_failed($action_type);
+                       },
+               }
+       }
+}
+
 impl ChannelManager {
        /// Constructs a new ChannelManager to hold several channels and route between them.
        ///
@@ -667,33 +685,6 @@ impl ChannelManager {
                }
        }
 
-       fn handle_monitor_update_fail(&self, mut channel_state_lock: MutexGuard<ChannelHolder>, channel_id: &[u8; 32], err: ChannelMonitorUpdateErr, reason: RAACommitmentOrder) {
-               match err {
-                       ChannelMonitorUpdateErr::PermanentFailure => {
-                               let mut chan = {
-                                       let channel_state = channel_state_lock.borrow_parts();
-                                       let chan = channel_state.by_id.remove(channel_id).expect("monitor_update_failed must be called within the same lock as the channel get!");
-                                       if let Some(short_id) = chan.get_short_channel_id() {
-                                               channel_state.short_to_id.remove(&short_id);
-                                       }
-                                       chan
-                               };
-                               mem::drop(channel_state_lock);
-                               self.finish_force_close_channel(chan.force_shutdown());
-                               if let Ok(update) = self.get_channel_update(&chan) {
-                                       let mut channel_state = self.channel_state.lock().unwrap();
-                                       channel_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
-                                               msg: update
-                                       });
-                               }
-                       },
-                       ChannelMonitorUpdateErr::TemporaryFailure => {
-                               let channel = channel_state_lock.by_id.get_mut(channel_id).expect("monitor_update_failed must be called within the same lock as the channel get!");
-                               channel.monitor_update_failed(reason);
-                       },
-               }
-       }
-
        #[inline]
        fn gen_rho_mu_from_shared_secret(shared_secret: &[u8]) -> ([u8; 32], [u8; 32]) {
                assert_eq!(shared_secret.len(), 32);
@@ -1193,7 +1184,17 @@ impl ChannelManager {
        /// May generate a SendHTLCs message event on success, which should be relayed.
        ///
        /// Raises APIError::RoutError when invalid route or forward parameter
-       /// (cltv_delta, fee, node public key) is specified
+       /// (cltv_delta, fee, node public key) is specified.
+       /// Raises APIError::ChannelUnavailable if the next-hop channel is not available for updates
+       /// (including due to previous monitor update failure or new permanent monitor update failure).
+       /// Raised APIError::MonitorUpdateFailed if a new monitor update failure prevented sending the
+       /// relevant updates.
+       ///
+       /// In case of APIError::RouteError/APIError::ChannelUnavailable, the payment send has failed
+       /// and you may wish to retry via a different route immediately.
+       /// In case of APIError::MonitorUpdateFailed, the commitment update has been irrevocably
+       /// committed on our end and we're just waiting for a monitor update to send it. Do NOT retry
+       /// the payment via a different route unless you intend to pay twice!
        pub fn send_payment(&self, route: Route, payment_hash: [u8; 32]) -> Result<(), APIError> {
                if route.hops.len() < 1 || route.hops.len() > 20 {
                        return Err(APIError::RouteError{err: "Route didn't go anywhere/had bogus size"});
@@ -1224,45 +1225,46 @@ impl ChannelManager {
                                Some(id) => id.clone(),
                        };
 
-                       match {
-                               let channel_state = channel_lock.borrow_parts();
-                               if let hash_map::Entry::Occupied(mut chan) = channel_state.by_id.entry(id) {
+                       let channel_state = channel_lock.borrow_parts();
+                       if let hash_map::Entry::Occupied(mut chan) = channel_state.by_id.entry(id) {
+                               match {
                                        if chan.get().get_their_node_id() != route.hops.first().unwrap().pubkey {
                                                return Err(APIError::RouteError{err: "Node ID mismatch on first hop!"});
                                        }
-                                       if chan.get().is_awaiting_monitor_update() {
-                                               return Err(APIError::MonitorUpdateFailed);
-                                       }
                                        if !chan.get().is_live() {
-                                               return Err(APIError::ChannelUnavailable{err: "Peer for first hop currently disconnected!"});
+                                               return Err(APIError::ChannelUnavailable{err: "Peer for first hop currently disconnected/pending monitor update!"});
                                        }
                                        break_chan_entry!(self, chan.get_mut().send_htlc_and_commit(htlc_msat, payment_hash.clone(), htlc_cltv, HTLCSource::OutboundRoute {
                                                route: route.clone(),
                                                session_priv: session_priv.clone(),
                                                first_hop_htlc_msat: htlc_msat,
                                        }, onion_packet), channel_state, chan)
-                               } else { unreachable!(); }
-                       } {
-                               Some((update_add, commitment_signed, chan_monitor)) => {
-                                       if let Err(e) = self.monitor.add_update_monitor(chan_monitor.get_funding_txo().unwrap(), chan_monitor) {
-                                               self.handle_monitor_update_fail(channel_lock, &id, e, RAACommitmentOrder::CommitmentFirst);
-                                               return Err(APIError::MonitorUpdateFailed);
-                                       }
+                               } {
+                                       Some((update_add, commitment_signed, chan_monitor)) => {
+                                               if let Err(e) = self.monitor.add_update_monitor(chan_monitor.get_funding_txo().unwrap(), chan_monitor) {
+                                                       maybe_break_monitor_err!(self, e, channel_state, chan, RAACommitmentOrder::CommitmentFirst);
+                                                       // Note that MonitorUpdateFailed here indicates (per function docs)
+                                                       // that we will resent the commitment update once we unfree monitor
+                                                       // updating, so we have to take special care that we don't return
+                                                       // something else in case we will resend later!
+                                                       return Err(APIError::MonitorUpdateFailed);
+                                               }
 
-                                       channel_lock.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
-                                               node_id: route.hops.first().unwrap().pubkey,
-                                               updates: msgs::CommitmentUpdate {
-                                                       update_add_htlcs: vec![update_add],
-                                                       update_fulfill_htlcs: Vec::new(),
-                                                       update_fail_htlcs: Vec::new(),
-                                                       update_fail_malformed_htlcs: Vec::new(),
-                                                       update_fee: None,
-                                                       commitment_signed,
-                                               },
-                                       });
-                               },
-                               None => {},
-                       }
+                                               channel_state.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
+                                                       node_id: route.hops.first().unwrap().pubkey,
+                                                       updates: msgs::CommitmentUpdate {
+                                                               update_add_htlcs: vec![update_add],
+                                                               update_fulfill_htlcs: Vec::new(),
+                                                               update_fail_htlcs: Vec::new(),
+                                                               update_fail_malformed_htlcs: Vec::new(),
+                                                               update_fee: None,
+                                                               commitment_signed,
+                                                       },
+                                               });
+                                       },
+                                       None => {},
+                               }
+                       } else { unreachable!(); }
                        return Ok(());
                };
 
@@ -1441,7 +1443,7 @@ impl ChannelManager {
                                                        },
                                                };
                                                if let Err(_e) = self.monitor.add_update_monitor(monitor.get_funding_txo().unwrap(), monitor) {
-                                                       unimplemented!();// but def dont push the event...
+                                                       unimplemented!();
                                                }
                                                channel_state.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
                                                        node_id: forward_chan.get_their_node_id(),
@@ -6946,15 +6948,19 @@ mod tests {
                let (_, payment_hash_1) = get_payment_preimage_hash!(nodes[0]);
 
                *nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::PermanentFailure);
-               if let Err(APIError::MonitorUpdateFailed) = nodes[0].node.send_payment(route, payment_hash_1) {} else { panic!(); }
+               if let Err(APIError::ChannelUnavailable {..}) = nodes[0].node.send_payment(route, payment_hash_1) {} else { panic!(); }
                check_added_monitors!(nodes[0], 1);
 
                let events_1 = nodes[0].node.get_and_clear_pending_msg_events();
-               assert_eq!(events_1.len(), 1);
+               assert_eq!(events_1.len(), 2);
                match events_1[0] {
                        MessageSendEvent::BroadcastChannelUpdate { .. } => {},
                        _ => panic!("Unexpected event"),
                };
+               match events_1[1] {
+                       MessageSendEvent::HandleError { node_id, .. } => assert_eq!(node_id, nodes[1].node.get_our_node_id()),
+                       _ => panic!("Unexpected event"),
+               };
 
                // TODO: Once we hit the chain with the failure transaction we should check that we get a
                // PaymentFailed event