From: Matt Corallo Date: Fri, 27 Jan 2023 23:01:39 +0000 (+0000) Subject: Move retry-limiting to `retry_payment_with_route` X-Git-Tag: v0.0.114-beta~32^2~2 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=8af05e0172ab8176d1abe139521fda9d6efbed9a;p=rust-lightning Move retry-limiting to `retry_payment_with_route` The documentation for `Retry` is very clear that it counts the number of failed paths, not discrete retries. When we added retries internally in `ChannelManager`, we switched to counting the number if discrete retries, even if multiple paths failed and were replace with multiple MPP HTLCs. Because we are now rewriting retries, we take this opportunity to reduce the places where retries are analyzed, specifically a good chunk of code is removed from `pay_internal`. Because we now retry multiple failed paths with one single retry, we keep the new behavior, updating the docs on `Retry` to describe the new behavior. --- diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 3b5861048..59d1dbfc9 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -2493,7 +2493,7 @@ where #[cfg(test)] pub(crate) fn test_add_new_pending_payment(&self, payment_hash: PaymentHash, payment_secret: Option, payment_id: PaymentId, route: &Route) -> Result, PaymentSendFailure> { let best_block_height = self.best_block.read().unwrap().height(); - self.pending_outbound_payments.test_add_new_pending_payment(payment_hash, payment_secret, payment_id, route, Retry::Attempts(0), &self.entropy_source, best_block_height) + self.pending_outbound_payments.test_add_new_pending_payment(payment_hash, payment_secret, payment_id, route, None, &self.entropy_source, best_block_height) } @@ -7361,7 +7361,7 @@ where hash_map::Entry::Vacant(entry) => { let path_fee = path.get_path_fees(); entry.insert(PendingOutboundPayment::Retryable { - retry_strategy: Retry::Attempts(0), + retry_strategy: None, attempts: PaymentAttempts::new(), route_params: None, session_privs: [session_priv_bytes].iter().map(|a| *a).collect(), diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 0ff7a33e9..08f600b2d 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -41,7 +41,7 @@ pub(crate) enum PendingOutboundPayment { session_privs: HashSet<[u8; 32]>, }, Retryable { - retry_strategy: Retry, + retry_strategy: Option, attempts: PaymentAttempts, route_params: Option, session_privs: HashSet<[u8; 32]>, @@ -82,11 +82,25 @@ impl PendingOutboundPayment { attempts.count += 1; } } + fn is_auto_retryable_now(&self) -> bool { + match self { + PendingOutboundPayment::Retryable { retry_strategy: Some(strategy), attempts, .. } => { + strategy.is_retryable_now(&attempts) + }, + _ => false, + } + } fn is_retryable_now(&self) -> bool { - if let PendingOutboundPayment::Retryable { retry_strategy, attempts, .. } = self { - return retry_strategy.is_retryable_now(&attempts) + match self { + PendingOutboundPayment::Retryable { retry_strategy: None, .. } => { + // We're handling retries manually, we can always retry. + true + }, + PendingOutboundPayment::Retryable { retry_strategy: Some(strategy), attempts, .. } => { + strategy.is_retryable_now(&attempts) + }, + _ => false, } - false } pub fn insert_previously_failed_scid(&mut self, scid: u64) { if let PendingOutboundPayment::Retryable { route_params: Some(params), .. } = self { @@ -212,9 +226,9 @@ impl PendingOutboundPayment { pub enum Retry { /// Max number of attempts to retry payment. /// - /// Note that this is the number of *path* failures, not full payment retries. For multi-path - /// payments, if this is less than the total number of paths, we will never even retry all of the - /// payment's paths. + /// Each attempt may be multiple HTLCs along multiple paths if the router decides to split up a + /// retry, and may retry multiple failed HTLCs at once if they failed around the same time and + /// were retried along a route from a single call to [`Router::find_route`]. Attempts(usize), #[cfg(not(feature = "no-std"))] /// Time elapsed before abandoning retries for a payment. @@ -409,7 +423,7 @@ impl OutboundPayments { F: Fn(&Vec, &Option, &PaymentHash, &Option, u64, u32, PaymentId, &Option, [u8; 32]) -> Result<(), APIError> { - let onion_session_privs = self.add_new_pending_payment(payment_hash, *payment_secret, payment_id, route, Retry::Attempts(0), None, entropy_source, best_block_height)?; + let onion_session_privs = self.add_new_pending_payment(payment_hash, *payment_secret, payment_id, route, None, None, entropy_source, best_block_height)?; self.pay_route_internal(route, payment_hash, payment_secret, None, payment_id, None, onion_session_privs, node_signer, best_block_height, &send_payment_along_path) .map_err(|e| { self.remove_outbound_if_all_failed(payment_id, &e); e }) @@ -430,7 +444,7 @@ impl OutboundPayments { None => 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, &route, Retry::Attempts(0), None, entropy_source, best_block_height)?; + let onion_session_privs = self.add_new_pending_payment(payment_hash, None, payment_id, &route, None, None, entropy_source, best_block_height)?; match self.pay_route_internal(route, payment_hash, &None, Some(preimage), payment_id, None, onion_session_privs, node_signer, best_block_height, &send_payment_along_path) { Ok(()) => Ok(payment_hash), @@ -459,11 +473,10 @@ impl OutboundPayments { let mut outbounds = self.pending_outbound_payments.lock().unwrap(); let mut retry_id_route_params = None; for (pmt_id, pmt) in outbounds.iter_mut() { - if pmt.is_retryable_now() { + if pmt.is_auto_retryable_now() { if let PendingOutboundPayment::Retryable { pending_amt_msat, total_msat, route_params: Some(params), .. } = pmt { if pending_amt_msat < total_msat { retry_id_route_params = Some((*pmt_id, params.clone())); - pmt.increment_attempts(); break } } @@ -509,35 +522,21 @@ impl OutboundPayments { }))?; let res = if let Some((payment_hash, payment_secret, retry_strategy)) = initial_send_info { - let onion_session_privs = self.add_new_pending_payment(payment_hash, *payment_secret, payment_id, &route, retry_strategy, Some(route_params.clone()), entropy_source, best_block_height)?; + let onion_session_privs = self.add_new_pending_payment(payment_hash, *payment_secret, payment_id, &route, Some(retry_strategy), Some(route_params.clone()), entropy_source, best_block_height)?; self.pay_route_internal(&route, payment_hash, payment_secret, None, payment_id, None, onion_session_privs, node_signer, best_block_height, send_payment_along_path) } else { self.retry_payment_with_route(&route, payment_id, entropy_source, node_signer, best_block_height, send_payment_along_path) }; match res { Err(PaymentSendFailure::AllFailedResendSafe(_)) => { - let mut outbounds = self.pending_outbound_payments.lock().unwrap(); - if let Some(payment) = outbounds.get_mut(&payment_id) { - let retryable = payment.is_retryable_now(); - if retryable { - payment.increment_attempts(); - } else { return res } - } else { return res } - core::mem::drop(outbounds); let retry_res = 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, "Result retrying payment id {}: {:?}", log_bytes!(payment_id.0), retry_res); + if let Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { err })) = &retry_res { + if err.starts_with("Retries exhausted ") { return res; } + } retry_res }, - Err(PaymentSendFailure::PartialFailure { failed_paths_retry: Some(retry), results, .. }) => { - let mut outbounds = self.pending_outbound_payments.lock().unwrap(); - if let Some(payment) = outbounds.get_mut(&payment_id) { - let retryable = payment.is_retryable_now(); - if retryable { - payment.increment_attempts(); - } else { return Err(PaymentSendFailure::PartialFailure { failed_paths_retry: Some(retry), results, payment_id }) } - } else { return Err(PaymentSendFailure::PartialFailure { failed_paths_retry: Some(retry), results, payment_id }) } - core::mem::drop(outbounds); - + Err(PaymentSendFailure::PartialFailure { failed_paths_retry: Some(retry), .. }) => { // Some paths were sent, even if we failed to send the full MPP value our recipient may // misbehave and claim the funds, at which point we have to consider the payment sent, so // return `Ok()` here, ignoring any retry errors. @@ -611,6 +610,12 @@ impl OutboundPayments { })); }, }; + if !payment.is_retryable_now() { + return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { + err: format!("Retries exhausted for payment id {}", log_bytes!(payment_id.0)), + })) + } + payment.increment_attempts(); for (path, session_priv_bytes) in route.paths.iter().zip(onion_session_privs.iter()) { assert!(payment.insert(*session_priv_bytes, path)); } @@ -646,7 +651,7 @@ impl OutboundPayments { } let route = Route { paths: vec![hops], payment_params: None }; - let onion_session_privs = self.add_new_pending_payment(payment_hash, None, payment_id, &route, Retry::Attempts(0), None, entropy_source, best_block_height)?; + let onion_session_privs = self.add_new_pending_payment(payment_hash, None, payment_id, &route, None, None, entropy_source, best_block_height)?; match self.pay_route_internal(&route, payment_hash, &None, None, payment_id, None, onion_session_privs, node_signer, best_block_height, &send_payment_along_path) { Ok(()) => Ok((payment_hash, payment_id)), @@ -660,14 +665,14 @@ impl OutboundPayments { #[cfg(test)] pub(super) fn test_add_new_pending_payment( &self, payment_hash: PaymentHash, payment_secret: Option, payment_id: PaymentId, - route: &Route, retry_strategy: Retry, entropy_source: &ES, best_block_height: u32 + route: &Route, retry_strategy: Option, entropy_source: &ES, best_block_height: u32 ) -> Result, PaymentSendFailure> where ES::Target: EntropySource { self.add_new_pending_payment(payment_hash, payment_secret, payment_id, route, retry_strategy, None, entropy_source, best_block_height) } pub(super) fn add_new_pending_payment( &self, payment_hash: PaymentHash, payment_secret: Option, payment_id: PaymentId, - route: &Route, retry_strategy: Retry, route_params: Option, + route: &Route, retry_strategy: Option, route_params: Option, entropy_source: &ES, best_block_height: u32 ) -> Result, PaymentSendFailure> where ES::Target: EntropySource { let mut onion_session_privs = Vec::with_capacity(route.paths.len()); @@ -969,7 +974,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_retryable_now(); + let is_retryable_now = payment.get().is_auto_retryable_now(); if let Some(scid) = short_channel_id { payment.get_mut().insert_previously_failed_scid(scid); } @@ -1110,7 +1115,7 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment, (0, session_privs, required), (1, pending_fee_msat, option), (2, payment_hash, required), - (not_written, retry_strategy, (static_value, Retry::Attempts(0))), + (not_written, retry_strategy, (static_value, None)), (4, payment_secret, option), (not_written, attempts, (static_value, PaymentAttempts::new())), (6, total_msat, required), @@ -1207,7 +1212,7 @@ mod tests { let err = if on_retry { outbound_payments.add_new_pending_payment(PaymentHash([0; 32]), None, PaymentId([0; 32]), - &Route { paths: vec![], payment_params: None }, Retry::Attempts(1), Some(route_params.clone()), + &Route { paths: vec![], payment_params: None }, Some(Retry::Attempts(1)), Some(route_params.clone()), &&keys_manager, 0).unwrap(); outbound_payments.pay_internal( PaymentId([0; 32]), None, route_params, &&router, vec![], InFlightHtlcs::new(), diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index c7c8198f6..b3acfc320 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -2251,7 +2251,7 @@ fn no_extra_retries_on_back_to_back_fail() { node_features: nodes[1].node.node_features(), short_channel_id: chan_1_scid, channel_features: nodes[1].node.channel_features(), - fee_msat: 0, + fee_msat: 0, // nodes[1] will fail the payment as we don't pay its fee cltv_expiry_delta: 100, }, RouteHop { pubkey: nodes[2].node.get_our_node_id(), @@ -2266,7 +2266,7 @@ fn no_extra_retries_on_back_to_back_fail() { node_features: nodes[1].node.node_features(), short_channel_id: chan_1_scid, channel_features: nodes[1].node.channel_features(), - fee_msat: 0, + fee_msat: 0, // nodes[1] will fail the payment as we don't pay its fee cltv_expiry_delta: 100, }, RouteHop { pubkey: nodes[2].node.get_our_node_id(), @@ -2280,10 +2280,11 @@ fn no_extra_retries_on_back_to_back_fail() { payment_params: Some(PaymentParameters::from_node_id(nodes[2].node.get_our_node_id(), TEST_FINAL_CLTV)), }; nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone())); - // On retry, we'll only be asked for one path - route.paths.remove(1); let mut second_payment_params = route_params.payment_params.clone(); second_payment_params.previously_failed_channels = vec![chan_2_scid, chan_2_scid]; + // On retry, we'll only return one path + route.paths.remove(1); + route.paths[0][1].fee_msat = amt_msat; nodes[0].router.expect_find_route(RouteParameters { payment_params: second_payment_params, final_value_msat: amt_msat, final_cltv_expiry_delta: TEST_FINAL_CLTV, @@ -2351,10 +2352,16 @@ fn no_extra_retries_on_back_to_back_fail() { // At this point A has sent two HTLCs which both failed due to lack of fee. It now has two // pending `PaymentPathFailed` events, one with `all_paths_failed` unset, and the second - // with it set. The first event will use up the only retry we are allowed, with the second - // `PaymentPathFailed` being passed up to the user (us, in this case). Previously, we'd - // treated this as "HTLC complete" and dropped the retry counter, causing us to retry again - // if the final HTLC failed. + // with it set. + // + // Previously, we retried payments in an event consumer, which would retry each + // `PaymentPathFailed` individually. In that setup, we had retried the payment in response to + // the first `PaymentPathFailed`, then seen the second `PaymentPathFailed` with + // `all_paths_failed` set and assumed the payment was completely failed. We ultimately fixed it + // by adding the `PaymentFailed` event. + // + // Because we now retry payments as a batch, we simply return a single-path route in the + // second, batched, request, have that fail, then complete the payment via `abandon_payment`. let mut events = nodes[0].node.get_and_clear_pending_events(); assert_eq!(events.len(), 4); match events[0] {