X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=blobdiff_plain;f=lightning%2Fsrc%2Fln%2Foutbound_payment.rs;h=6ebd7bc547fe42b1db2242b89ca249ca514c8ae3;hb=685b08d8c13c62a3a4c4cf283c3d86b96fd3de23;hp=3a49d7c2be6403da603369a63aa2876e4b5163f1;hpb=6b49af1563ffdd2533900059922bd655c1f90a8d;p=rust-lightning diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 3a49d7c2..6ebd7bc5 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -163,13 +163,13 @@ impl PendingOutboundPayment { let our_payment_hash; core::mem::swap(&mut session_privs, match self { PendingOutboundPayment::Legacy { .. } | - PendingOutboundPayment::Fulfilled { .. } => + PendingOutboundPayment::Fulfilled { .. } => return Err(()), - PendingOutboundPayment::Retryable { session_privs, payment_hash, .. } | - PendingOutboundPayment::Abandoned { session_privs, payment_hash, .. } => { - our_payment_hash = *payment_hash; - session_privs - }, + PendingOutboundPayment::Retryable { session_privs, payment_hash, .. } | + PendingOutboundPayment::Abandoned { session_privs, payment_hash, .. } => { + our_payment_hash = *payment_hash; + session_privs + }, }); *self = PendingOutboundPayment::Abandoned { session_privs, payment_hash: our_payment_hash }; Ok(()) @@ -323,68 +323,58 @@ pub enum PaymentSendFailure { /// /// You can freely resend the payment in full (with the parameter error fixed). /// - /// Because the payment failed outright, no payment tracking is done, you do not need to call - /// [`ChannelManager::abandon_payment`] and [`ChannelManager::retry_payment`] will *not* work - /// for this payment. + /// Because the payment failed outright, no payment tracking is done and no + /// [`Event::PaymentPathFailed`] or [`Event::PaymentFailed`] events will be generated. /// - /// [`ChannelManager::abandon_payment`]: crate::ln::channelmanager::ChannelManager::abandon_payment - /// [`ChannelManager::retry_payment`]: crate::ln::channelmanager::ChannelManager::retry_payment + /// [`Event::PaymentPathFailed`]: crate::util::events::Event::PaymentPathFailed + /// [`Event::PaymentFailed`]: crate::util::events::Event::PaymentFailed ParameterError(APIError), /// A parameter in a single path which was passed to send_payment was invalid, preventing us /// from attempting to send the payment at all. /// /// You can freely resend the payment in full (with the parameter error fixed). /// + /// Because the payment failed outright, no payment tracking is done and no + /// [`Event::PaymentPathFailed`] or [`Event::PaymentFailed`] events will be generated. + /// /// The results here are ordered the same as the paths in the route object which was passed to /// send_payment. /// - /// Because the payment failed outright, no payment tracking is done, you do not need to call - /// [`ChannelManager::abandon_payment`] and [`ChannelManager::retry_payment`] will *not* work - /// for this payment. - /// - /// [`ChannelManager::abandon_payment`]: crate::ln::channelmanager::ChannelManager::abandon_payment - /// [`ChannelManager::retry_payment`]: crate::ln::channelmanager::ChannelManager::retry_payment + /// [`Event::PaymentPathFailed`]: crate::util::events::Event::PaymentPathFailed + /// [`Event::PaymentFailed`]: crate::util::events::Event::PaymentFailed PathParameterError(Vec>), /// All paths which were attempted failed to send, with no channel state change taking place. /// You can freely resend the payment in full (though you probably want to do so over different /// paths than the ones selected). /// - /// Because the payment failed outright, no payment tracking is done, you do not need to call - /// [`ChannelManager::abandon_payment`] and [`ChannelManager::retry_payment`] will *not* work - /// for this payment. + /// Because the payment failed outright, no payment tracking is done and no + /// [`Event::PaymentPathFailed`] or [`Event::PaymentFailed`] events will be generated. /// - /// [`ChannelManager::abandon_payment`]: crate::ln::channelmanager::ChannelManager::abandon_payment - /// [`ChannelManager::retry_payment`]: crate::ln::channelmanager::ChannelManager::retry_payment + /// [`Event::PaymentPathFailed`]: crate::util::events::Event::PaymentPathFailed + /// [`Event::PaymentFailed`]: crate::util::events::Event::PaymentFailed AllFailedResendSafe(Vec), /// Indicates that a payment for the provided [`PaymentId`] is already in-flight and has not - /// yet completed (i.e. generated an [`Event::PaymentSent`]) or been abandoned (via - /// [`ChannelManager::abandon_payment`]). + /// yet completed (i.e. generated an [`Event::PaymentSent`] or [`Event::PaymentFailed`]). /// /// [`PaymentId`]: crate::ln::channelmanager::PaymentId /// [`Event::PaymentSent`]: crate::util::events::Event::PaymentSent - /// [`ChannelManager::abandon_payment`]: crate::ln::channelmanager::ChannelManager::abandon_payment + /// [`Event::PaymentFailed`]: crate::util::events::Event::PaymentFailed DuplicatePayment, - /// 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 - /// in over-/re-payment. + /// Some paths that were attempted failed to send, though some paths may have succeeded. At least + /// some paths have irrevocably committed to the HTLC. /// - /// The results here are ordered the same as the paths in the route object which was passed to - /// send_payment, and any `Err`s which are not [`APIError::MonitorUpdateInProgress`] can be - /// safely retried via [`ChannelManager::retry_payment`]. + /// The results here are ordered the same as the paths in the route object that was passed to + /// send_payment. /// - /// Any entries which contain `Err(APIError::MonitorUpdateInprogress)` or `Ok(())` MUST NOT be - /// retried as they will result in over-/re-payment. These HTLCs all either successfully sent - /// (in the case of `Ok(())`) or will send once a [`MonitorEvent::Completed`] is provided for - /// the next-hop channel with the latest update_id. + /// Any entries that contain `Err(APIError::MonitorUpdateInprogress)` will send once a + /// [`MonitorEvent::Completed`] is provided for the next-hop channel with the latest update_id. /// - /// [`ChannelManager::retry_payment`]: crate::ln::channelmanager::ChannelManager::retry_payment /// [`MonitorEvent::Completed`]: crate::chain::channelmonitor::MonitorEvent::Completed PartialFailure { - /// The errors themselves, in the same order as the route hops. + /// The errors themselves, in the same order as the paths from the route. results: Vec>, /// If some paths failed without irrevocably committing to the new HTLC(s), this will - /// contain a [`RouteParameters`] object which can be used to calculate a new route that - /// will pay all remaining unpaid balance. + /// contain a [`RouteParameters`] object for the failing paths. failed_paths_retry: Option, /// The payment id for the payment, which is now at least partially pending. payment_id: PaymentId, @@ -393,31 +383,34 @@ pub enum PaymentSendFailure { pub(super) struct OutboundPayments { pub(super) pending_outbound_payments: Mutex>, + pub(super) retry_lock: Mutex<()>, } impl OutboundPayments { pub(super) fn new() -> Self { Self { - pending_outbound_payments: Mutex::new(HashMap::new()) + pending_outbound_payments: Mutex::new(HashMap::new()), + retry_lock: Mutex::new(()), } } - pub(super) fn send_payment( + pub(super) fn send_payment( &self, payment_hash: PaymentHash, payment_secret: &Option, payment_id: PaymentId, retry_strategy: Retry, route_params: RouteParameters, router: &R, - first_hops: Vec, inflight_htlcs: InFlightHtlcs, entropy_source: &ES, - node_signer: &NS, best_block_height: u32, logger: &L, send_payment_along_path: F, + first_hops: Vec, compute_inflight_htlcs: IH, entropy_source: &ES, + node_signer: &NS, best_block_height: u32, logger: &L, send_payment_along_path: SP, ) -> Result<(), PaymentSendFailure> where R::Target: Router, ES::Target: EntropySource, NS::Target: NodeSigner, L::Target: Logger, - F: Fn(&Vec, &Option, &PaymentHash, &Option, u64, + IH: Fn() -> InFlightHtlcs, + SP: Fn(&Vec, &Option, &PaymentHash, &Option, u64, u32, PaymentId, &Option, [u8; 32]) -> Result<(), APIError>, { self.pay_internal(payment_id, Some((payment_hash, payment_secret, None, retry_strategy)), - route_params, router, first_hops, inflight_htlcs, entropy_source, node_signer, + route_params, router, first_hops, &compute_inflight_htlcs, entropy_source, node_signer, best_block_height, logger, &send_payment_along_path) .map_err(|e| { self.remove_outbound_if_all_failed(payment_id, &e); e }) } @@ -439,27 +432,26 @@ impl OutboundPayments { .map_err(|e| { self.remove_outbound_if_all_failed(payment_id, &e); e }) } - pub(super) fn send_spontaneous_payment( + pub(super) fn send_spontaneous_payment( &self, payment_preimage: Option, payment_id: PaymentId, retry_strategy: Retry, route_params: RouteParameters, router: &R, - first_hops: Vec, inflight_htlcs: InFlightHtlcs, entropy_source: &ES, - node_signer: &NS, best_block_height: u32, logger: &L, send_payment_along_path: F + first_hops: Vec, inflight_htlcs: IH, entropy_source: &ES, + node_signer: &NS, best_block_height: u32, logger: &L, send_payment_along_path: SP ) -> Result where R::Target: Router, ES::Target: EntropySource, NS::Target: NodeSigner, L::Target: Logger, - F: Fn(&Vec, &Option, &PaymentHash, &Option, u64, + IH: Fn() -> InFlightHtlcs, + SP: Fn(&Vec, &Option, &PaymentHash, &Option, u64, u32, PaymentId, &Option, [u8; 32]) -> Result<(), APIError>, { - let preimage = match payment_preimage { - Some(p) => p, - None => PaymentPreimage(entropy_source.get_secure_random_bytes()), - }; + let preimage = payment_preimage + .unwrap_or_else(|| PaymentPreimage(entropy_source.get_secure_random_bytes())); let payment_hash = PaymentHash(Sha256::hash(&preimage.0).into_inner()); self.pay_internal(payment_id, Some((payment_hash, &None, Some(preimage), retry_strategy)), - route_params, router, first_hops, inflight_htlcs, entropy_source, node_signer, + route_params, router, first_hops, &inflight_htlcs, entropy_source, node_signer, best_block_height, logger, &send_payment_along_path) .map(|()| payment_hash) .map_err(|e| { self.remove_outbound_if_all_failed(payment_id, &e); e }) @@ -475,10 +467,8 @@ impl OutboundPayments { F: Fn(&Vec, &Option, &PaymentHash, &Option, u64, u32, PaymentId, &Option, [u8; 32]) -> Result<(), APIError> { - let preimage = match payment_preimage { - Some(p) => p, - None => PaymentPreimage(entropy_source.get_secure_random_bytes()), - }; + let preimage = payment_preimage + .unwrap_or_else(|| PaymentPreimage(entropy_source.get_secure_random_bytes())); let payment_hash = PaymentHash(Sha256::hash(&preimage.0).into_inner()); let onion_session_privs = self.add_new_pending_payment(payment_hash, None, payment_id, Some(preimage), &route, None, None, entropy_source, best_block_height)?; @@ -493,7 +483,8 @@ impl OutboundPayments { pub(super) fn check_retry_payments( &self, router: &R, first_hops: FH, inflight_htlcs: IH, entropy_source: &ES, node_signer: &NS, - best_block_height: u32, logger: &L, send_payment_along_path: SP, + best_block_height: u32, pending_events: &Mutex>, logger: &L, + send_payment_along_path: SP, ) where R::Target: Router, @@ -505,6 +496,7 @@ impl OutboundPayments { FH: Fn() -> Vec, L::Target: Logger, { + let _single_thread = self.retry_lock.lock().unwrap(); loop { let mut outbounds = self.pending_outbound_payments.lock().unwrap(); let mut retry_id_route_params = None; @@ -527,28 +519,48 @@ impl OutboundPayments { } } } + core::mem::drop(outbounds); if let Some((payment_id, route_params)) = retry_id_route_params { - core::mem::drop(outbounds); - if let Err(e) = self.pay_internal(payment_id, None, route_params, router, first_hops(), inflight_htlcs(), entropy_source, node_signer, best_block_height, logger, &send_payment_along_path) { + if let Err(e) = self.pay_internal(payment_id, None, route_params, router, first_hops(), &inflight_htlcs, entropy_source, node_signer, best_block_height, logger, &send_payment_along_path) { log_info!(logger, "Errored retrying payment: {:?}", e); + // If we error on retry, there is no chance of the payment succeeding and no HTLCs have + // been irrevocably committed to, so we can safely abandon. + self.abandon_payment(payment_id, pending_events); } } else { break } } + + let mut outbounds = self.pending_outbound_payments.lock().unwrap(); + outbounds.retain(|pmt_id, pmt| { + let mut retain = true; + if !pmt.is_auto_retryable_now() && pmt.remaining_parts() == 0 { + if pmt.mark_abandoned().is_ok() { + pending_events.lock().unwrap().push(events::Event::PaymentFailed { + payment_id: *pmt_id, + payment_hash: pmt.payment_hash().expect("PendingOutboundPayments::Retryable always has a payment hash set"), + }); + retain = false; + } + } + retain + }); } - fn pay_internal( + /// Will return `Ok(())` iff at least one HTLC is sent for the payment. + fn pay_internal( &self, payment_id: PaymentId, initial_send_info: Option<(PaymentHash, &Option, Option, Retry)>, route_params: RouteParameters, router: &R, first_hops: Vec, - inflight_htlcs: InFlightHtlcs, entropy_source: &ES, node_signer: &NS, best_block_height: u32, - logger: &L, send_payment_along_path: &F, + inflight_htlcs: &IH, entropy_source: &ES, node_signer: &NS, best_block_height: u32, + logger: &L, send_payment_along_path: &SP, ) -> Result<(), PaymentSendFailure> where R::Target: Router, ES::Target: EntropySource, NS::Target: NodeSigner, L::Target: Logger, - F: Fn(&Vec, &Option, &PaymentHash, &Option, u64, + IH: Fn() -> InFlightHtlcs, + SP: Fn(&Vec, &Option, &PaymentHash, &Option, u64, u32, PaymentId, &Option, [u8; 32]) -> Result<(), APIError> { #[cfg(feature = "std")] { @@ -561,7 +573,7 @@ impl OutboundPayments { let route = router.find_route( &node_signer.get_node_id(Recipient::Node).unwrap(), &route_params, - Some(&first_hops.iter().collect::>()), &inflight_htlcs + Some(&first_hops.iter().collect::>()), &inflight_htlcs(), ).map_err(|e| PaymentSendFailure::ParameterError(APIError::APIMisuseError { err: format!("Failed to find a route for payment {}: {:?}", log_bytes!(payment_id.0), e), // TODO: add APIError::RouteNotFound }))?; @@ -877,8 +889,8 @@ impl OutboundPayments { .map_err(|e| { self.remove_outbound_if_all_failed(payment_id, &e); e }) } - // 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`. + // If we failed to send any paths, remove the new PaymentId from the `pending_outbound_payments` + // map as the payment is free to be resent. fn remove_outbound_if_all_failed(&self, payment_id: PaymentId, err: &PaymentSendFailure) { if let &PaymentSendFailure::AllFailedResendSafe(_) = err { let removed = self.pending_outbound_payments.lock().unwrap().remove(&payment_id).is_some(); @@ -1005,6 +1017,7 @@ impl OutboundPayments { #[cfg(not(test))] let (network_update, short_channel_id, payment_retryable, _, _) = onion_error.decode_onion_failure(secp_ctx, logger, &source); + let payment_is_probe = payment_is_probe(payment_hash, &payment_id, probing_cookie_secret); let mut session_priv_bytes = [0; 32]; session_priv_bytes.copy_from_slice(&session_priv[..]); let mut outbounds = self.pending_outbound_payments.lock().unwrap(); @@ -1021,7 +1034,7 @@ impl OutboundPayments { log_trace!(logger, "Received failure of HTLC with payment_hash {} after payment completion", log_bytes!(payment_hash.0)); return } - let is_retryable_now = payment.get().is_auto_retryable_now(); + let mut is_retryable_now = payment.get().is_auto_retryable_now(); if let Some(scid) = short_channel_id { payment.get_mut().insert_previously_failed_scid(scid); } @@ -1052,13 +1065,19 @@ impl OutboundPayments { }); } + if payment_is_probe || !is_retryable_now || !payment_retryable || retry.is_none() { + let _ = payment.get_mut().mark_abandoned(); // we'll only Err if it's a legacy payment + is_retryable_now = false; + } if payment.get().remaining_parts() == 0 { all_paths_failed = true; if payment.get().abandoned() { - full_failure_ev = Some(events::Event::PaymentFailed { - payment_id: *payment_id, - payment_hash: payment.get().payment_hash().expect("PendingOutboundPayments::RetriesExceeded always has a payment hash set"), - }); + if !payment_is_probe { + full_failure_ev = Some(events::Event::PaymentFailed { + payment_id: *payment_id, + payment_hash: payment.get().payment_hash().expect("PendingOutboundPayments::RetriesExceeded always has a payment hash set"), + }); + } payment.remove(); } } @@ -1071,7 +1090,7 @@ impl OutboundPayments { log_trace!(logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0)); let path_failure = { - if payment_is_probe(payment_hash, &payment_id, probing_cookie_secret) { + if payment_is_probe { if !payment_retryable { events::Event::ProbeSuccessful { payment_id: *payment_id, @@ -1093,7 +1112,9 @@ impl OutboundPayments { if let Some(scid) = short_channel_id { retry.as_mut().map(|r| r.payment_params.previously_failed_channels.push(scid)); } - if payment_retryable && attempts_remaining && retry.is_some() { + // If we miss abandoning the payment above, we *must* generate an event here or else the + // payment will sit in our outbounds forever. + if attempts_remaining { debug_assert!(full_failure_ev.is_none()); pending_retry_ev = Some(events::Event::PendingHTLCsForwardable { time_forwardable: Duration::from_millis(MIN_HTLC_RELAY_HOLDING_CELL_MILLIS), @@ -1121,13 +1142,14 @@ impl OutboundPayments { if let Some(ev) = pending_retry_ev { pending_events.push(ev); } } - pub(super) fn abandon_payment(&self, payment_id: PaymentId) -> Option { - let mut failed_ev = None; + pub(super) fn abandon_payment( + &self, payment_id: PaymentId, pending_events: &Mutex> + ) { let mut outbounds = self.pending_outbound_payments.lock().unwrap(); if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) { if let Ok(()) = payment.get_mut().mark_abandoned() { if payment.get().remaining_parts() == 0 { - failed_ev = Some(events::Event::PaymentFailed { + pending_events.lock().unwrap().push(events::Event::PaymentFailed { payment_id, payment_hash: payment.get().payment_hash().expect("PendingOutboundPayments::RetriesExceeded always has a payment hash set"), }); @@ -1135,7 +1157,6 @@ impl OutboundPayments { } } } - failed_ev } #[cfg(test)] @@ -1206,7 +1227,7 @@ mod tests { use crate::ln::outbound_payment::{OutboundPayments, Retry}; use crate::routing::gossip::NetworkGraph; use crate::routing::router::{InFlightHtlcs, PaymentParameters, Route, RouteParameters}; - use crate::sync::Arc; + use crate::sync::{Arc, Mutex}; use crate::util::errors::APIError; use crate::util::test_utils; @@ -1222,7 +1243,8 @@ mod tests { let logger = test_utils::TestLogger::new(); let genesis_hash = genesis_block(Network::Testnet).header.block_hash(); let network_graph = Arc::new(NetworkGraph::new(genesis_hash, &logger)); - let router = test_utils::TestRouter::new(network_graph); + let scorer = Mutex::new(test_utils::TestScorer::new()); + let router = test_utils::TestRouter::new(network_graph, &scorer); let secp_ctx = Secp256k1::new(); let keys_manager = test_utils::TestKeysInterface::new(&[0; 32], Network::Testnet); @@ -1238,12 +1260,12 @@ mod tests { }; let err = if on_retry { outbound_payments.pay_internal( - PaymentId([0; 32]), None, expired_route_params, &&router, vec![], InFlightHtlcs::new(), + PaymentId([0; 32]), None, expired_route_params, &&router, vec![], &|| InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger, &|_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err() } else { outbound_payments.send_payment( PaymentHash([0; 32]), &None, PaymentId([0; 32]), Retry::Attempts(0), expired_route_params, - &&router, vec![], InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger, + &&router, vec![], || InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger, |_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err() }; if let PaymentSendFailure::ParameterError(APIError::APIMisuseError { err }) = err { @@ -1261,7 +1283,8 @@ mod tests { let logger = test_utils::TestLogger::new(); let genesis_hash = genesis_block(Network::Testnet).header.block_hash(); let network_graph = Arc::new(NetworkGraph::new(genesis_hash, &logger)); - let router = test_utils::TestRouter::new(network_graph); + let scorer = Mutex::new(test_utils::TestScorer::new()); + let router = test_utils::TestRouter::new(network_graph, &scorer); let secp_ctx = Secp256k1::new(); let keys_manager = test_utils::TestKeysInterface::new(&[0; 32], Network::Testnet); @@ -1280,12 +1303,12 @@ mod tests { &Route { paths: vec![], payment_params: None }, Some(Retry::Attempts(1)), Some(route_params.payment_params.clone()), &&keys_manager, 0).unwrap(); outbound_payments.pay_internal( - PaymentId([0; 32]), None, route_params, &&router, vec![], InFlightHtlcs::new(), + PaymentId([0; 32]), None, route_params, &&router, vec![], &|| InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger, &|_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err() } else { outbound_payments.send_payment( PaymentHash([0; 32]), &None, PaymentId([0; 32]), Retry::Attempts(0), route_params, - &&router, vec![], InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger, + &&router, vec![], || InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger, |_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err() }; if let PaymentSendFailure::ParameterError(APIError::APIMisuseError { err }) = err {