Clean up forward_/claimable_htlcs handling and document consistency
[rust-lightning] / src / ln / channelmanager.rs
index 63059074de707caf4953866c139cbce6e0951b2c..45abf69604b54deca266b4fd17a3c26593189d92 100644 (file)
@@ -115,14 +115,19 @@ struct ChannelHolder {
        short_to_id: HashMap<u64, [u8; 32]>,
        next_forward: Instant,
        /// short channel id -> forward infos. Key of 0 means payments received
+       /// Note that while this is held in the same mutex as the channels themselves, no consistency
+       /// guarantees are made about there existing a channel with the short id here, nor the short
+       /// ids in the PendingForwardHTLCInfo!
        forward_htlcs: HashMap<u64, Vec<PendingForwardHTLCInfo>>,
+       /// Note that while this is held in the same mutex as the channels themselves, no consistency
+       /// guarantees are made about the channels given here actually existing anymore by the time you
+       /// go to read them!
        claimable_htlcs: HashMap<[u8; 32], PendingOutboundHTLC>,
 }
 struct MutChannelHolder<'a> {
        by_id: &'a mut HashMap<[u8; 32], Channel>,
        short_to_id: &'a mut HashMap<u64, [u8; 32]>,
        next_forward: &'a mut Instant,
-       /// short channel id -> forward infos. Key of 0 means payments received
        forward_htlcs: &'a mut HashMap<u64, Vec<PendingForwardHTLCInfo>>,
        claimable_htlcs: &'a mut HashMap<[u8; 32], PendingOutboundHTLC>,
 }
@@ -636,6 +641,12 @@ impl ChannelManager {
        /// Sends a payment along a given route.
        /// Value parameters are provided via the last hop in route, see documentation for RouteHop
        /// fields for more info.
+       /// Note that if the payment_hash already exists elsewhere (eg you're sending a duplicative
+       /// payment), we don't do anything to stop you! We always try to ensure that if the provided
+       /// next hop knows the preimage to payment_hash they can claim an additional amount as
+       /// specified in the last hop in the route! Thus, you should probably do your own
+       /// payment_preimage tracking (which you should already be doing as they represent "proof of
+       /// payment") and prevent double-sends yourself.
        /// See-also docs on Channel::send_htlc_and_commit.
        /// May generate a SendHTLCs event on success, which should be relayed.
        pub fn send_payment(&self, route: Route, payment_hash: [u8; 32]) -> Result<(), HandleError> {
@@ -662,29 +673,33 @@ impl ChannelManager {
                let onion_packet = ChannelManager::construct_onion_packet(onion_payloads, onion_keys, &payment_hash)?;
 
                let (first_hop_node_id, (update_add, commitment_signed, chan_monitor)) = {
-                       let mut channel_state = self.channel_state.lock().unwrap();
+                       let mut channel_state_lock = self.channel_state.lock().unwrap();
+                       let channel_state = channel_state_lock.borrow_parts();
+
                        let id = match channel_state.short_to_id.get(&route.hops.first().unwrap().short_channel_id) {
                                None => return Err(HandleError{err: "No channel available with first hop!", action: None}),
                                Some(id) => id.clone()
                        };
+
+                       let claimable_htlc_entry = channel_state.claimable_htlcs.entry(payment_hash.clone());
+                       if let hash_map::Entry::Occupied(_) = claimable_htlc_entry {
+                               return Err(HandleError{err: "Already had pending HTLC with the same payment_hash", action: None});
+                       }
+
                        let res = {
                                let chan = channel_state.by_id.get_mut(&id).unwrap();
                                if chan.get_their_node_id() != route.hops.first().unwrap().pubkey {
                                        return Err(HandleError{err: "Node ID mismatch on first hop!", action: None});
                                }
-                               chan.send_htlc_and_commit(htlc_msat, payment_hash.clone(), htlc_cltv, onion_packet)?
+                               chan.send_htlc_and_commit(htlc_msat, payment_hash, htlc_cltv, onion_packet)?
                        };
 
                        let first_hop_node_id = route.hops.first().unwrap().pubkey;
 
-                       if channel_state.claimable_htlcs.insert(payment_hash, PendingOutboundHTLC::OutboundRoute {
+                       claimable_htlc_entry.or_insert(PendingOutboundHTLC::OutboundRoute {
                                route,
                                session_priv,
-                       }).is_some() {
-                               // TODO: We need to track these better, we're not generating these, so a
-                               // third-party might make this happen:
-                               panic!("payment_hash was repeated! Don't let this happen");
-                       }
+                       });
 
                        match res {
                                Some(msgs) => (first_hop_node_id, msgs),
@@ -874,6 +889,12 @@ impl ChannelManager {
                self.fail_htlc_backwards_internal(self.channel_state.lock().unwrap(), payment_hash, HTLCFailReason::Reason { failure_code: 0x4000 | 15, data: Vec::new() })
        }
 
+       /// Fails an HTLC backwards to the sender of it to us.
+       /// Note that while we take a channel_state lock as input, we do *not* assume consistency here.
+       /// There are several callsites that do stupid things like loop over a list of payment_hashes
+       /// to fail and take the channel_state lock for each iteration (as we take ownership and may
+       /// drop it). In other words, no assumptions are made that entries in claimable_htlcs point to
+       /// still-available channels.
        fn fail_htlc_backwards_internal(&self, mut channel_state: MutexGuard<ChannelHolder>, payment_hash: &[u8; 32], onion_error: HTLCFailReason) -> bool {
                let mut pending_htlc = {
                        match channel_state.claimable_htlcs.remove(payment_hash) {
@@ -894,7 +915,7 @@ impl ChannelManager {
                }
 
                match pending_htlc {
-                       PendingOutboundHTLC::CycledRoute { .. } => { panic!("WAT"); },
+                       PendingOutboundHTLC::CycledRoute { .. } => unreachable!(),
                        PendingOutboundHTLC::OutboundRoute { .. } => {
                                mem::drop(channel_state);
 
@@ -990,7 +1011,7 @@ impl ChannelManager {
                }
 
                match pending_htlc {
-                       PendingOutboundHTLC::CycledRoute { .. } => { panic!("WAT"); },
+                       PendingOutboundHTLC::CycledRoute { .. } => unreachable!(),
                        PendingOutboundHTLC::OutboundRoute { .. } => {
                                if from_user {
                                        panic!("Called claim_funds with a preimage for an outgoing payment. There is nothing we can do with this, and something is seriously wrong if you knew this...");
@@ -1006,13 +1027,20 @@ impl ChannelManager {
                                let (node_id, fulfill_msgs) = {
                                        let chan_id = match channel_state.short_to_id.get(&source_short_channel_id) {
                                                Some(chan_id) => chan_id.clone(),
-                                               None => return false
+                                               None => {
+                                                       // TODO: There is probably a channel manager somewhere that needs to
+                                                       // learn the preimage as the channel already hit the chain and that's
+                                                       // why its missing.
+                                                       return false
+                                               }
                                        };
 
                                        let chan = channel_state.by_id.get_mut(&chan_id).unwrap();
                                        match chan.get_update_fulfill_htlc_and_commit(payment_preimage) {
                                                Ok(msg) => (chan.get_their_node_id(), msg),
                                                Err(_e) => {
+                                                       // TODO: There is probably a channel manager somewhere that needs to
+                                                       // learn the preimage as the channel may be about to hit the chain.
                                                        //TODO: Do something with e?
                                                        return false;
                                                },
@@ -1020,20 +1048,19 @@ impl ChannelManager {
                                };
 
                                mem::drop(channel_state);
-                               match fulfill_msgs {
-                                       Some((msg, commitment_msg, chan_monitor)) => {
-                                               if let Err(_e) = self.monitor.add_update_monitor(chan_monitor.get_funding_txo().unwrap(), chan_monitor) {
-                                                       unimplemented!();// but def dont push the event...
-                                               }
+                               if let Some(chan_monitor) = fulfill_msgs.1 {
+                                       if let Err(_e) = self.monitor.add_update_monitor(chan_monitor.get_funding_txo().unwrap(), chan_monitor) {
+                                               unimplemented!();// but def dont push the event...
+                                       }
+                               }
 
-                                               let mut pending_events = self.pending_events.lock().unwrap();
-                                               pending_events.push(events::Event::SendFulfillHTLC {
-                                                       node_id: node_id,
-                                                       msg,
-                                                       commitment_msg,
-                                               });
-                                       },
-                                       None => {},
+                               if let Some((msg, commitment_msg)) = fulfill_msgs.0 {
+                                       let mut pending_events = self.pending_events.lock().unwrap();
+                                       pending_events.push(events::Event::SendFulfillHTLC {
+                                               node_id: node_id,
+                                               msg,
+                                               commitment_msg,
+                                       });
                                }
                                true
                        },
@@ -1562,7 +1589,7 @@ impl ChannelMessageHandler for ChannelManager {
                                pending_forward_info.prev_short_channel_id = short_channel_id;
                                (short_channel_id, chan.update_add_htlc(&msg, pending_forward_info)?)
                        },
-                       None => return Err(HandleError{err: "Failed to find corresponding channel", action: None}), //TODO: panic?
+                       None => return Err(HandleError{err: "Failed to find corresponding channel", action: None}),
                };
 
                match claimable_htlcs_entry {
@@ -1572,7 +1599,7 @@ impl ChannelMessageHandler for ChannelManager {
                                        &mut PendingOutboundHTLC::OutboundRoute { ref route, ref session_priv } => {
                                                (route.clone(), session_priv.clone())
                                        },
-                                       _ => { panic!("WAT") },
+                                       _ => unreachable!(),
                                };
                                *outbound_route = PendingOutboundHTLC::CycledRoute {
                                        source_short_channel_id,