Allow users to specify the `PaymentId` for new outbound payments
[rust-lightning] / lightning / src / ln / channelmanager.rs
index e3128d15f531ebfb94a6caf1bc386e57380190d3..c223a74131c9ecbc7c07f27ae5a24838fa2137cc 100644 (file)
@@ -488,12 +488,6 @@ pub(crate) enum PendingOutboundPayment {
 }
 
 impl PendingOutboundPayment {
-       fn is_retryable(&self) -> bool {
-               match self {
-                       PendingOutboundPayment::Retryable { .. } => true,
-                       _ => false,
-               }
-       }
        fn is_fulfilled(&self) -> bool {
                match self {
                        PendingOutboundPayment::Fulfilled { .. } => true,
@@ -1206,6 +1200,9 @@ pub enum PaymentSendFailure {
        /// All paths which were attempted failed to send, with no channel state change taking place.
        /// You can freely retry the payment in full (though you probably want to do so over different
        /// paths than the ones selected).
+       ///
+       /// [`ChannelManager::abandon_payment`] does *not* need to be called for this payment and
+       /// [`ChannelManager::retry_payment`] will *not* work for this payment.
        AllFailedRetrySafe(Vec<APIError>),
        /// Some paths which were attempted failed to send, though possibly not all. At least some
        /// paths have irrevocably committed to the HTLC and retrying the payment in full would result
@@ -2434,10 +2431,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
        }
 
        // Only public for testing, this should otherwise never be called direcly
-       pub(crate) fn send_payment_along_path(&self, path: &Vec<RouteHop>, payment_params: &Option<PaymentParameters>, payment_hash: &PaymentHash, payment_secret: &Option<PaymentSecret>, total_value: u64, cur_height: u32, payment_id: PaymentId, keysend_preimage: &Option<PaymentPreimage>) -> Result<(), APIError> {
+       pub(crate) fn send_payment_along_path(&self, path: &Vec<RouteHop>, payment_params: &Option<PaymentParameters>, payment_hash: &PaymentHash, payment_secret: &Option<PaymentSecret>, total_value: u64, cur_height: u32, payment_id: PaymentId, keysend_preimage: &Option<PaymentPreimage>, session_priv_bytes: [u8; 32]) -> Result<(), APIError> {
                log_trace!(self.logger, "Attempting to send payment for path with next hop {}", path.first().unwrap().short_channel_id);
                let prng_seed = self.keys_manager.get_secure_random_bytes();
-               let session_priv_bytes = self.keys_manager.get_secure_random_bytes();
                let session_priv = SecretKey::from_slice(&session_priv_bytes[..]).expect("RNG is busted");
 
                let onion_keys = onion_utils::construct_onion_keys(&self.secp_ctx, &path, &session_priv)
@@ -2453,36 +2449,11 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                let err: Result<(), _> = loop {
                        let mut channel_lock = self.channel_state.lock().unwrap();
 
-                       let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap();
-                       let payment_entry = pending_outbounds.entry(payment_id);
-                       if let hash_map::Entry::Occupied(payment) = &payment_entry {
-                               if !payment.get().is_retryable() {
-                                       return Err(APIError::RouteError {
-                                               err: "Payment already completed"
-                                       });
-                               }
-                       }
-
                        let id = match channel_lock.short_to_chan_info.get(&path.first().unwrap().short_channel_id) {
                                None => return Err(APIError::ChannelUnavailable{err: "No channel available with first hop!".to_owned()}),
                                Some((_cp_id, chan_id)) => chan_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 {
@@ -2511,9 +2482,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                                RAACommitmentOrder::CommitmentFirst, false, true))
                                                {
                                                        (ChannelMonitorUpdateStatus::PermanentFailure, Err(e)) => break Err(e),
-                                                       (ChannelMonitorUpdateStatus::Completed, Ok(())) => {
-                                                               insert_outbound_payment!();
-                                                       },
+                                                       (ChannelMonitorUpdateStatus::Completed, Ok(())) => {},
                                                        (ChannelMonitorUpdateStatus::InProgress, Err(_)) => {
                                                                // Note that MonitorUpdateInProgress here indicates (per function
                                                                // docs) that we will resend the commitment update once monitor
@@ -2521,7 +2490,6 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                                // indicating that it is unsafe to retry the payment wholesale,
                                                                // which we do in the send_payment check for
                                                                // MonitorUpdateInProgress, below.
-                                                               insert_outbound_payment!(); // Only do this after possibly break'ing on Perm failure above.
                                                                return Err(APIError::MonitorUpdateInProgress);
                                                        },
                                                        _ => unreachable!(),
@@ -2540,7 +2508,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                        },
                                                });
                                        },
-                                       None => { insert_outbound_payment!(); },
+                                       None => { },
                                }
                        } else { unreachable!(); }
                        return Ok(());
@@ -2559,14 +2527,20 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
        /// 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.
+       /// If a pending payment is currently in-flight with the same [`PaymentId`] provided, this
+       /// method will error with an [`APIError::RouteError`]. Note, however, that once a payment
+       /// is no longer pending (either via [`ChannelManager::abandon_payment`], or handling of an
+       /// [`Event::PaymentSent`]) LDK will not stop you from sending a second payment with the same
+       /// [`PaymentId`].
        ///
-       /// May generate SendHTLCs message(s) event on success, which should be relayed.
+       /// Thus, in order to ensure duplicate payments are not sent, you should implement your own
+       /// tracking of payments, including state to indicate once a payment has completed. Because you
+       /// should also ensure that [`PaymentHash`]es are not re-used, for simplicity, you should
+       /// consider using the [`PaymentHash`] as the key for tracking payments. In that case, the
+       /// [`PaymentId`] should be a copy of the [`PaymentHash`] bytes.
+       ///
+       /// May generate SendHTLCs message(s) event on success, which should be relayed (e.g. via
+       /// [`PeerManager::process_events`]).
        ///
        /// Each path may have a different return value, and PaymentSendValue may return a Vec with
        /// each entry matching the corresponding-index entry in the route paths, see
@@ -2590,14 +2564,55 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
        /// newer nodes, it will be provided to you in the invoice. If you do not have one, the Route
        /// must not contain multiple paths as multi-path payments require a recipient-provided
        /// payment_secret.
+       ///
        /// If a payment_secret *is* provided, we assume that the invoice had the payment_secret feature
        /// bit set (either as required or as available). If multiple paths are present in the Route,
        /// we assume the invoice had the basic_mpp feature set.
-       pub fn send_payment(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>) -> Result<PaymentId, PaymentSendFailure> {
-               self.send_payment_internal(route, payment_hash, payment_secret, None, None, None)
+       ///
+       /// [`Event::PaymentSent`]: events::Event::PaymentSent
+       /// [`PeerManager::process_events`]: crate::ln::peer_handler::PeerManager::process_events
+       pub fn send_payment(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>, payment_id: PaymentId) -> Result<(), PaymentSendFailure> {
+               let onion_session_privs = self.add_new_pending_payment(payment_hash, *payment_secret, payment_id, route)?;
+               self.send_payment_internal(route, payment_hash, payment_secret, None, payment_id, None, onion_session_privs)
+       }
+
+       #[cfg(test)]
+       pub(crate) fn test_add_new_pending_payment(&self, payment_hash: PaymentHash, payment_secret: Option<PaymentSecret>, payment_id: PaymentId, route: &Route) -> Result<Vec<[u8; 32]>, PaymentSendFailure> {
+               self.add_new_pending_payment(payment_hash, payment_secret, payment_id, route)
        }
 
-       fn send_payment_internal(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>, keysend_preimage: Option<PaymentPreimage>, payment_id: Option<PaymentId>, recv_value_msat: Option<u64>) -> Result<PaymentId, PaymentSendFailure> {
+       fn add_new_pending_payment(&self, payment_hash: PaymentHash, payment_secret: Option<PaymentSecret>, payment_id: PaymentId, route: &Route) -> Result<Vec<[u8; 32]>, PaymentSendFailure> {
+               let mut onion_session_privs = Vec::with_capacity(route.paths.len());
+               for _ in 0..route.paths.len() {
+                       onion_session_privs.push(self.keys_manager.get_secure_random_bytes());
+               }
+
+               let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap();
+               match pending_outbounds.entry(payment_id) {
+                       hash_map::Entry::Occupied(_) => Err(PaymentSendFailure::ParameterError(APIError::RouteError {
+                               err: "Payment already in progress"
+                       })),
+                       hash_map::Entry::Vacant(entry) => {
+                               let payment = entry.insert(PendingOutboundPayment::Retryable {
+                                       session_privs: HashSet::new(),
+                                       pending_amt_msat: 0,
+                                       pending_fee_msat: Some(0),
+                                       payment_hash,
+                                       payment_secret,
+                                       starting_block_height: self.best_block.read().unwrap().height(),
+                                       total_msat: route.get_total_amount(),
+                               });
+
+                               for (path, session_priv_bytes) in route.paths.iter().zip(onion_session_privs.iter()) {
+                                       assert!(payment.insert(*session_priv_bytes, path));
+                               }
+
+                               Ok(onion_session_privs)
+                       },
+               }
+       }
+
+       fn send_payment_internal(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>, keysend_preimage: Option<PaymentPreimage>, payment_id: PaymentId, recv_value_msat: Option<u64>, onion_session_privs: Vec<[u8; 32]>) -> Result<(), PaymentSendFailure> {
                if route.paths.len() < 1 {
                        return Err(PaymentSendFailure::ParameterError(APIError::RouteError{err: "There must be at least one path to send over"}));
                }
@@ -2607,7 +2622,6 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                let mut total_value = 0;
                let our_node_id = self.get_our_node_id();
                let mut path_errs = Vec::with_capacity(route.paths.len());
-               let payment_id = if let Some(id) = payment_id { id } else { PaymentId(self.keys_manager.get_secure_random_bytes()) };
                'path_check: for path in route.paths.iter() {
                        if path.len() < 1 || path.len() > 20 {
                                path_errs.push(Err(APIError::RouteError{err: "Path didn't go anywhere/had bogus size"}));
@@ -2632,8 +2646,28 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 
                let cur_height = self.best_block.read().unwrap().height() + 1;
                let mut results = Vec::new();
-               for path in route.paths.iter() {
-                       results.push(self.send_payment_along_path(&path, &route.payment_params, &payment_hash, payment_secret, total_value, cur_height, payment_id, &keysend_preimage));
+               debug_assert_eq!(route.paths.len(), onion_session_privs.len());
+               for (path, session_priv) in route.paths.iter().zip(onion_session_privs.into_iter()) {
+                       let mut path_res = self.send_payment_along_path(&path, &route.payment_params, &payment_hash, payment_secret, total_value, cur_height, payment_id, &keysend_preimage, session_priv);
+                       match path_res {
+                               Ok(_) => {},
+                               Err(APIError::MonitorUpdateInProgress) => {
+                                       // While a MonitorUpdateInProgress is an Err(_), the payment is still
+                                       // considered "in flight" and we shouldn't remove it from the
+                                       // PendingOutboundPayment set.
+                               },
+                               Err(_) => {
+                                       let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap();
+                                       if let Some(payment) = pending_outbounds.get_mut(&payment_id) {
+                                               let removed = payment.remove(&session_priv, Some(path));
+                                               debug_assert!(removed, "This can't happen as the payment has an entry for this path added by callers");
+                                       } else {
+                                               debug_assert!(false, "This can't happen as the payment was added by callers");
+                                               path_res = Err(APIError::APIMisuseError { err: "Internal error: payment disappeared during processing. Please report this bug!".to_owned() });
+                                       }
+                               }
+                       }
+                       results.push(path_res);
                }
                let mut has_ok = false;
                let mut has_err = false;
@@ -2667,12 +2701,13 @@ 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());
+                       // If we failed to send any paths, we should remove the new PaymentId from the
+                       // `pending_outbound_payments` map, as the user isn't expected to `abandon_payment`.
+                       let removed = self.pending_outbound_payments.lock().unwrap().remove(&payment_id).is_some();
+                       debug_assert!(removed, "We should always have a pending payment to remove here");
                        Err(PaymentSendFailure::AllFailedRetrySafe(results.drain(..).map(|r| r.unwrap_err()).collect()))
                } else {
-                       Ok(payment_id)
+                       Ok(())
                }
        }
 
@@ -2696,44 +2731,55 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                        }
                }
 
+               let mut onion_session_privs = Vec::with_capacity(route.paths.len());
+               for _ in 0..route.paths.len() {
+                       onion_session_privs.push(self.keys_manager.get_secure_random_bytes());
+               }
+
                let (total_msat, payment_hash, payment_secret) = {
-                       let outbounds = self.pending_outbound_payments.lock().unwrap();
-                       if let Some(payment) = outbounds.get(&payment_id) {
-                               match payment {
-                                       PendingOutboundPayment::Retryable {
-                                               total_msat, payment_hash, payment_secret, pending_amt_msat, ..
-                                       } => {
-                                               let retry_amt_msat: u64 = route.paths.iter().map(|path| path.last().unwrap().fee_msat).sum();
-                                               if retry_amt_msat + *pending_amt_msat > *total_msat * (100 + RETRY_OVERFLOW_PERCENTAGE) / 100 {
+                       let mut outbounds = self.pending_outbound_payments.lock().unwrap();
+                       match outbounds.get_mut(&payment_id) {
+                               Some(payment) => {
+                                       let res = match payment {
+                                               PendingOutboundPayment::Retryable {
+                                                       total_msat, payment_hash, payment_secret, pending_amt_msat, ..
+                                               } => {
+                                                       let retry_amt_msat: u64 = route.paths.iter().map(|path| path.last().unwrap().fee_msat).sum();
+                                                       if retry_amt_msat + *pending_amt_msat > *total_msat * (100 + RETRY_OVERFLOW_PERCENTAGE) / 100 {
+                                                               return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError {
+                                                                       err: format!("retry_amt_msat of {} will put pending_amt_msat (currently: {}) more than 10% over total_payment_amt_msat of {}", retry_amt_msat, pending_amt_msat, total_msat).to_string()
+                                                               }))
+                                                       }
+                                                       (*total_msat, *payment_hash, *payment_secret)
+                                               },
+                                               PendingOutboundPayment::Legacy { .. } => {
                                                        return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError {
-                                                               err: format!("retry_amt_msat of {} will put pending_amt_msat (currently: {}) more than 10% over total_payment_amt_msat of {}", retry_amt_msat, pending_amt_msat, total_msat).to_string()
+                                                               err: "Unable to retry payments that were initially sent on LDK versions prior to 0.0.102".to_string()
                                                        }))
-                                               }
-                                               (*total_msat, *payment_hash, *payment_secret)
-                                       },
-                                       PendingOutboundPayment::Legacy { .. } => {
-                                               return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError {
-                                                       err: "Unable to retry payments that were initially sent on LDK versions prior to 0.0.102".to_string()
-                                               }))
-                                       },
-                                       PendingOutboundPayment::Fulfilled { .. } => {
-                                               return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError {
-                                                       err: "Payment already completed".to_owned()
-                                               }));
-                                       },
-                                       PendingOutboundPayment::Abandoned { .. } => {
-                                               return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError {
-                                                       err: "Payment already abandoned (with some HTLCs still pending)".to_owned()
-                                               }));
-                                       },
-                               }
-                       } else {
-                               return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError {
-                                       err: format!("Payment with ID {} not found", log_bytes!(payment_id.0)),
-                               }))
+                                               },
+                                               PendingOutboundPayment::Fulfilled { .. } => {
+                                                       return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError {
+                                                               err: "Payment already completed".to_owned()
+                                                       }));
+                                               },
+                                               PendingOutboundPayment::Abandoned { .. } => {
+                                                       return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError {
+                                                               err: "Payment already abandoned (with some HTLCs still pending)".to_owned()
+                                                       }));
+                                               },
+                                       };
+                                       for (path, session_priv_bytes) in route.paths.iter().zip(onion_session_privs.iter()) {
+                                               assert!(payment.insert(*session_priv_bytes, path));
+                                       }
+                                       res
+                               },
+                               None =>
+                                       return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError {
+                                               err: format!("Payment with ID {} not found", log_bytes!(payment_id.0)),
+                                       })),
                        }
                };
-               return self.send_payment_internal(route, payment_hash, &payment_secret, None, Some(payment_id), Some(total_msat)).map(|_| ())
+               self.send_payment_internal(route, payment_hash, &payment_secret, None, payment_id, Some(total_msat), onion_session_privs)
        }
 
        /// Signals that no further retries for the given payment will occur.
@@ -2773,7 +2819,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
        /// would be able to guess -- otherwise, an intermediate node may claim the payment and it will
        /// never reach the recipient.
        ///
-       /// See [`send_payment`] documentation for more details on the return value of this function.
+       /// See [`send_payment`] documentation for more details on the return value of this function
+       /// and idempotency guarantees provided by the [`PaymentId`] key.
        ///
        /// Similar to regular payments, you MUST NOT reuse a `payment_preimage` value. See
        /// [`send_payment`] for more information about the risks of duplicate preimage usage.
@@ -2781,14 +2828,16 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
        /// Note that `route` must have exactly one path.
        ///
        /// [`send_payment`]: Self::send_payment
-       pub fn send_spontaneous_payment(&self, route: &Route, payment_preimage: Option<PaymentPreimage>) -> Result<(PaymentHash, PaymentId), PaymentSendFailure> {
+       pub fn send_spontaneous_payment(&self, route: &Route, payment_preimage: Option<PaymentPreimage>, payment_id: PaymentId) -> Result<PaymentHash, PaymentSendFailure> {
                let preimage = match payment_preimage {
                        Some(p) => p,
                        None => PaymentPreimage(self.keys_manager.get_secure_random_bytes()),
                };
                let payment_hash = PaymentHash(Sha256::hash(&preimage.0).into_inner());
-               match self.send_payment_internal(route, payment_hash, &None, Some(preimage), None, None) {
-                       Ok(payment_id) => Ok((payment_hash, payment_id)),
+               let onion_session_privs = self.add_new_pending_payment(payment_hash, None, payment_id, &route)?;
+
+               match self.send_payment_internal(route, payment_hash, &None, Some(preimage), payment_id, None, onion_session_privs) {
+                       Ok(()) => Ok(payment_hash),
                        Err(e) => Err(e)
                }
        }
@@ -2808,9 +2857,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                }
 
                let route = Route { paths: vec![hops], payment_params: None };
+               let onion_session_privs = self.add_new_pending_payment(payment_hash, None, payment_id, &route)?;
 
-               match self.send_payment_internal(&route, payment_hash, &None, None, Some(payment_id), None) {
-                       Ok(payment_id) => Ok((payment_hash, payment_id)),
+               match self.send_payment_internal(&route, payment_hash, &None, None, payment_id, None, onion_session_privs) {
+                       Ok(()) => Ok((payment_hash, payment_id)),
                        Err(e) => Err(e)
                }
        }
@@ -7406,18 +7456,22 @@ mod tests {
 
                // First, send a partial MPP payment.
                let (route, our_payment_hash, payment_preimage, payment_secret) = get_route_and_payment_hash!(&nodes[0], nodes[1], 100_000);
+               let mut mpp_route = route.clone();
+               mpp_route.paths.push(mpp_route.paths[0].clone());
+
                let payment_id = PaymentId([42; 32]);
                // Use the utility function send_payment_along_path to send the payment with MPP data which
                // indicates there are more HTLCs coming.
                let cur_height = CHAN_CONFIRM_DEPTH + 1; // route_payment calls send_payment, which adds 1 to the current height. So we do the same here to match.
-               nodes[0].node.send_payment_along_path(&route.paths[0], &route.payment_params, &our_payment_hash, &Some(payment_secret), 200_000, cur_height, payment_id, &None).unwrap();
+               let session_privs = nodes[0].node.add_new_pending_payment(our_payment_hash, Some(payment_secret), payment_id, &mpp_route).unwrap();
+               nodes[0].node.send_payment_along_path(&mpp_route.paths[0], &route.payment_params, &our_payment_hash, &Some(payment_secret), 200_000, cur_height, payment_id, &None, session_privs[0]).unwrap();
                check_added_monitors!(nodes[0], 1);
                let mut events = nodes[0].node.get_and_clear_pending_msg_events();
                assert_eq!(events.len(), 1);
                pass_along_path(&nodes[0], &[&nodes[1]], 200_000, our_payment_hash, Some(payment_secret), events.drain(..).next().unwrap(), false, None);
 
                // Next, send a keysend payment with the same payment_hash and make sure it fails.
-               nodes[0].node.send_spontaneous_payment(&route, Some(payment_preimage)).unwrap();
+               nodes[0].node.send_spontaneous_payment(&route, Some(payment_preimage), PaymentId(payment_preimage.0)).unwrap();
                check_added_monitors!(nodes[0], 1);
                let mut events = nodes[0].node.get_and_clear_pending_msg_events();
                assert_eq!(events.len(), 1);
@@ -7440,7 +7494,7 @@ mod tests {
                expect_payment_failed!(nodes[0], our_payment_hash, true);
 
                // Send the second half of the original MPP payment.
-               nodes[0].node.send_payment_along_path(&route.paths[0], &route.payment_params, &our_payment_hash, &Some(payment_secret), 200_000, cur_height, payment_id, &None).unwrap();
+               nodes[0].node.send_payment_along_path(&mpp_route.paths[1], &route.payment_params, &our_payment_hash, &Some(payment_secret), 200_000, cur_height, payment_id, &None, session_privs[1]).unwrap();
                check_added_monitors!(nodes[0], 1);
                let mut events = nodes[0].node.get_and_clear_pending_msg_events();
                assert_eq!(events.len(), 1);
@@ -7538,7 +7592,7 @@ mod tests {
                        &nodes[0].node.get_our_node_id(), &route_params, &nodes[0].network_graph,
                        None, nodes[0].logger, &scorer, &random_seed_bytes
                ).unwrap();
-               nodes[0].node.send_spontaneous_payment(&route, Some(payment_preimage)).unwrap();
+               nodes[0].node.send_spontaneous_payment(&route, Some(payment_preimage), PaymentId(payment_preimage.0)).unwrap();
                check_added_monitors!(nodes[0], 1);
                let mut events = nodes[0].node.get_and_clear_pending_msg_events();
                assert_eq!(events.len(), 1);
@@ -7571,7 +7625,7 @@ mod tests {
                        &nodes[0].node.get_our_node_id(), &route_params, &nodes[0].network_graph,
                        None, nodes[0].logger, &scorer, &random_seed_bytes
                ).unwrap();
-               let (payment_hash, _) = nodes[0].node.send_spontaneous_payment(&route, Some(payment_preimage)).unwrap();
+               let payment_hash = nodes[0].node.send_spontaneous_payment(&route, Some(payment_preimage), PaymentId(payment_preimage.0)).unwrap();
                check_added_monitors!(nodes[0], 1);
                let mut events = nodes[0].node.get_and_clear_pending_msg_events();
                assert_eq!(events.len(), 1);
@@ -7581,7 +7635,7 @@ mod tests {
 
                // Next, attempt a regular payment and make sure it fails.
                let payment_secret = PaymentSecret([43; 32]);
-               nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap();
+               nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret), PaymentId(payment_hash.0)).unwrap();
                check_added_monitors!(nodes[0], 1);
                let mut events = nodes[0].node.get_and_clear_pending_msg_events();
                assert_eq!(events.len(), 1);
@@ -7624,7 +7678,7 @@ mod tests {
                let _chan = create_chan_between_nodes(&nodes[0], &nodes[1], channelmanager::provided_init_features(), channelmanager::provided_init_features());
                let route_params = RouteParameters {
                        payment_params: PaymentParameters::for_keysend(payee_pubkey),
-                       final_value_msat: 10000,
+                       final_value_msat: 10_000,
                        final_cltv_expiry_delta: 40,
                };
                let network_graph = nodes[0].network_graph;
@@ -7638,7 +7692,8 @@ mod tests {
 
                let test_preimage = PaymentPreimage([42; 32]);
                let mismatch_payment_hash = PaymentHash([43; 32]);
-               let _ = nodes[0].node.send_payment_internal(&route, mismatch_payment_hash, &None, Some(test_preimage), None, None).unwrap();
+               let session_privs = nodes[0].node.add_new_pending_payment(mismatch_payment_hash, None, PaymentId(mismatch_payment_hash.0), &route).unwrap();
+               nodes[0].node.send_payment_internal(&route, mismatch_payment_hash, &None, Some(test_preimage), PaymentId(mismatch_payment_hash.0), None, session_privs).unwrap();
                check_added_monitors!(nodes[0], 1);
 
                let updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
@@ -7668,7 +7723,7 @@ mod tests {
                let _chan = create_chan_between_nodes(&nodes[0], &nodes[1], channelmanager::provided_init_features(), channelmanager::provided_init_features());
                let route_params = RouteParameters {
                        payment_params: PaymentParameters::for_keysend(payee_pubkey),
-                       final_value_msat: 10000,
+                       final_value_msat: 10_000,
                        final_cltv_expiry_delta: 40,
                };
                let network_graph = nodes[0].network_graph;
@@ -7683,7 +7738,8 @@ mod tests {
                let test_preimage = PaymentPreimage([42; 32]);
                let test_secret = PaymentSecret([43; 32]);
                let payment_hash = PaymentHash(Sha256::hash(&test_preimage.0).into_inner());
-               let _ = nodes[0].node.send_payment_internal(&route, payment_hash, &Some(test_secret), Some(test_preimage), None, None).unwrap();
+               let session_privs = nodes[0].node.add_new_pending_payment(payment_hash, Some(test_secret), PaymentId(payment_hash.0), &route).unwrap();
+               nodes[0].node.send_payment_internal(&route, payment_hash, &Some(test_secret), Some(test_preimage), PaymentId(payment_hash.0), None, session_privs).unwrap();
                check_added_monitors!(nodes[0], 1);
 
                let updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
@@ -7720,7 +7776,7 @@ mod tests {
                route.paths[1][0].short_channel_id = chan_2_id;
                route.paths[1][1].short_channel_id = chan_4_id;
 
-               match nodes[0].node.send_payment(&route, payment_hash, &None).unwrap_err() {
+               match nodes[0].node.send_payment(&route, payment_hash, &None, PaymentId(payment_hash.0)).unwrap_err() {
                        PaymentSendFailure::ParameterError(APIError::APIMisuseError { ref err }) => {
                                assert!(regex::Regex::new(r"Payment secret is required for multi-path payments").unwrap().is_match(err))                        },
                        _ => panic!("unexpected error")
@@ -7875,7 +7931,7 @@ pub mod bench {
        use crate::chain::Listen;
        use crate::chain::chainmonitor::{ChainMonitor, Persist};
        use crate::chain::keysinterface::{KeysManager, KeysInterface, InMemorySigner};
-       use crate::ln::channelmanager::{self, BestBlock, ChainParameters, ChannelManager, PaymentHash, PaymentPreimage};
+       use crate::ln::channelmanager::{self, BestBlock, ChainParameters, ChannelManager, PaymentHash, PaymentPreimage, PaymentId};
        use crate::ln::functional_test_utils::*;
        use crate::ln::msgs::{ChannelMessageHandler, Init};
        use crate::routing::gossip::NetworkGraph;
@@ -8002,7 +8058,7 @@ pub mod bench {
                                let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0[..]).into_inner());
                                let payment_secret = $node_b.create_inbound_payment_for_hash(payment_hash, None, 7200).unwrap();
 
-                               $node_a.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap();
+                               $node_a.send_payment(&route, payment_hash, &Some(payment_secret), PaymentId(payment_hash.0)).unwrap();
                                let payment_event = SendEvent::from_event($node_a.get_and_clear_pending_msg_events().pop().unwrap());
                                $node_b.handle_update_add_htlc(&$node_a.get_our_node_id(), &payment_event.msgs[0]);
                                $node_b.handle_commitment_signed(&$node_a.get_our_node_id(), &payment_event.commitment_msg);