Merge pull request #1166 from TheBlueMatt/2021-11-chan-size-scoring
[rust-lightning] / lightning / src / ln / channelmanager.rs
index a5f84abfdb3e2865505db43149fbdebde96ee71c..03522e24ce9742dce47dcb274f12682854eed802 100644 (file)
@@ -1959,17 +1959,24 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                break Some(("Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta", 0x1000 | 13, Some(self.get_channel_update_for_unicast(chan).unwrap())));
                                        }
                                        let cur_height = self.best_block.read().unwrap().height() + 1;
-                                       // Theoretically, channel counterparty shouldn't send us a HTLC expiring now, but we want to be robust wrt to counterparty
-                                       // packet sanitization (see HTLC_FAIL_BACK_BUFFER rational)
+                                       // Theoretically, channel counterparty shouldn't send us a HTLC expiring now,
+                                       // but we want to be robust wrt to counterparty packet sanitization (see
+                                       // HTLC_FAIL_BACK_BUFFER rationale).
                                        if msg.cltv_expiry <= cur_height + HTLC_FAIL_BACK_BUFFER as u32 { // expiry_too_soon
                                                break Some(("CLTV expiry is too close", 0x1000 | 14, Some(self.get_channel_update_for_unicast(chan).unwrap())));
                                        }
                                        if msg.cltv_expiry > cur_height + CLTV_FAR_FAR_AWAY as u32 { // expiry_too_far
                                                break Some(("CLTV expiry is too far in the future", 21, None));
                                        }
-                                       // In theory, we would be safe against unintentional channel-closure, if we only required a margin of LATENCY_GRACE_PERIOD_BLOCKS.
-                                       // But, to be safe against policy reception, we use a longer delay.
-                                       if (*outgoing_cltv_value) as u64 <= (cur_height + HTLC_FAIL_BACK_BUFFER) as u64 {
+                                       // If the HTLC expires ~now, don't bother trying to forward it to our
+                                       // counterparty. They should fail it anyway, but we don't want to bother with
+                                       // the round-trips or risk them deciding they definitely want the HTLC and
+                                       // force-closing to ensure they get it if we're offline.
+                                       // We previously had a much more aggressive check here which tried to ensure
+                                       // our counterparty receives an HTLC which has *our* risk threshold met on it,
+                                       // but there is no need to do that, and since we're a bit conservative with our
+                                       // risk threshold it just results in failing to forward payments.
+                                       if (*outgoing_cltv_value) as u64 <= (cur_height + LATENCY_GRACE_PERIOD_BLOCKS) as u64 {
                                                break Some(("Outgoing CLTV value is too soon", 0x1000 | 14, Some(self.get_channel_update_for_unicast(chan).unwrap())));
                                        }
 
@@ -2085,6 +2092,21 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                Some(id) => id.clone(),
                        };
 
+                       macro_rules! insert_outbound_payment {
+                               () => {
+                                       let payment = payment_entry.or_insert_with(|| PendingOutboundPayment::Retryable {
+                                               session_privs: HashSet::new(),
+                                               pending_amt_msat: 0,
+                                               pending_fee_msat: Some(0),
+                                               payment_hash: *payment_hash,
+                                               payment_secret: *payment_secret,
+                                               starting_block_height: self.best_block.read().unwrap().height(),
+                                               total_msat: total_value,
+                                       });
+                                       assert!(payment.insert(session_priv_bytes, path));
+                               }
+                       }
+
                        let channel_state = &mut *channel_lock;
                        if let hash_map::Entry::Occupied(mut chan) = channel_state.by_id.entry(id) {
                                match {
@@ -2094,7 +2116,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                        if !chan.get().is_live() {
                                                return Err(APIError::ChannelUnavailable{err: "Peer for first hop currently disconnected/pending monitor update!".to_owned()});
                                        }
-                                       let send_res = break_chan_entry!(self, chan.get_mut().send_htlc_and_commit(
+                                       break_chan_entry!(self, chan.get_mut().send_htlc_and_commit(
                                                htlc_msat, payment_hash.clone(), htlc_cltv, HTLCSource::OutboundRoute {
                                                        path: path.clone(),
                                                        session_priv: session_priv.clone(),
@@ -2103,20 +2125,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                        payment_secret: payment_secret.clone(),
                                                        payee: payee.clone(),
                                                }, onion_packet, &self.logger),
-                                       channel_state, chan);
-
-                                       let payment = payment_entry.or_insert_with(|| PendingOutboundPayment::Retryable {
-                                               session_privs: HashSet::new(),
-                                               pending_amt_msat: 0,
-                                               pending_fee_msat: Some(0),
-                                               payment_hash: *payment_hash,
-                                               payment_secret: *payment_secret,
-                                               starting_block_height: self.best_block.read().unwrap().height(),
-                                               total_msat: total_value,
-                                       });
-                                       assert!(payment.insert(session_priv_bytes, path));
-
-                                       send_res
+                                       channel_state, chan)
                                } {
                                        Some((update_add, commitment_signed, monitor_update)) => {
                                                if let Err(e) = self.chain_monitor.update_channel(chan.get().get_funding_txo().unwrap(), monitor_update) {
@@ -2126,8 +2135,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                        // is restored. Therefore, we must return an error indicating that
                                                        // it is unsafe to retry the payment wholesale, which we do in the
                                                        // send_payment check for MonitorUpdateFailed, below.
+                                                       insert_outbound_payment!(); // Only do this after possibly break'ing on Perm failure above.
                                                        return Err(APIError::MonitorUpdateFailed);
                                                }
+                                               insert_outbound_payment!();
 
                                                log_debug!(self.logger, "Sending payment along path resulted in a commitment_signed for channel {}", log_bytes!(chan.get().channel_id()));
                                                channel_state.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
@@ -2142,7 +2153,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                        },
                                                });
                                        },
-                                       None => {},
+                                       None => { insert_outbound_payment!(); },
                                }
                        } else { unreachable!(); }
                        return Ok(());
@@ -2275,6 +2286,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                } else { None },
                        })
                } else if has_err {
+                       // If we failed to send any paths, we shouldn't have inserted the new PaymentId into
+                       // our `pending_outbound_payments` map at all.
+                       debug_assert!(self.pending_outbound_payments.lock().unwrap().get(&payment_id).is_none());
                        Err(PaymentSendFailure::AllFailedRetrySafe(results.drain(..).map(|r| r.unwrap_err()).collect()))
                } else {
                        Ok(payment_id)
@@ -6538,7 +6552,7 @@ pub mod bench {
        use ln::msgs::{ChannelMessageHandler, Init};
        use routing::network_graph::NetworkGraph;
        use routing::router::{Payee, get_route};
-       use routing::scorer::Scorer;
+       use routing::scoring::Scorer;
        use util::test_utils;
        use util::config::UserConfig;
        use util::events::{Event, MessageSendEvent, MessageSendEventsProvider, PaymentPurpose};