From 1224dac862ad5fa994efeb0830c0b8db88e22647 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Thu, 16 Feb 2023 16:40:51 -0500 Subject: [PATCH] On initial send retries, avoid previously failed scids Previously, we could have tried the same failed channels over and over until retries are exhausted. --- lightning/src/ln/outbound_payment.rs | 22 ++++++++++++---------- lightning/src/ln/payment_tests.rs | 20 +++++++++++++------- 2 files changed, 25 insertions(+), 17 deletions(-) diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 2e2dc6b40..a28169032 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -735,8 +735,8 @@ impl OutboundPayments { fn handle_pay_route_err( &self, err: PaymentSendFailure, payment_id: PaymentId, payment_hash: PaymentHash, route: Route, - route_params: RouteParameters, router: &R, first_hops: Vec, inflight_htlcs: &IH, - entropy_source: &ES, node_signer: &NS, best_block_height: u32, logger: &L, + mut route_params: RouteParameters, router: &R, first_hops: Vec, + inflight_htlcs: &IH, entropy_source: &ES, node_signer: &NS, best_block_height: u32, logger: &L, pending_events: &Mutex>, send_payment_along_path: &SP, ) where @@ -750,11 +750,11 @@ impl OutboundPayments { { match err { PaymentSendFailure::AllFailedResendSafe(errs) => { - Self::push_payment_path_failed_evs(payment_id, payment_hash, route.paths, errs.into_iter().map(|e| Err(e)), pending_events); + Self::push_path_failed_evs_and_scids(payment_id, payment_hash, &mut route_params, route.paths, errs.into_iter().map(|e| Err(e)), pending_events); self.retry_payment_internal(payment_id, route_params, router, first_hops, inflight_htlcs, entropy_source, node_signer, best_block_height, logger, pending_events, send_payment_along_path); }, - PaymentSendFailure::PartialFailure { failed_paths_retry: Some(retry), results, .. } => { - Self::push_payment_path_failed_evs(payment_id, payment_hash, route.paths, results.into_iter(), pending_events); + PaymentSendFailure::PartialFailure { failed_paths_retry: Some(mut retry), results, .. } => { + Self::push_path_failed_evs_and_scids(payment_id, payment_hash, &mut retry, route.paths, results.into_iter(), pending_events); // 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. @@ -766,7 +766,7 @@ impl OutboundPayments { // initial HTLC-Add messages yet. }, PaymentSendFailure::PathParameterError(results) => { - Self::push_payment_path_failed_evs(payment_id, payment_hash, route.paths, results.into_iter(), pending_events); + Self::push_path_failed_evs_and_scids(payment_id, payment_hash, &mut route_params, route.paths, results.into_iter(), pending_events); self.abandon_payment(payment_id, pending_events); }, PaymentSendFailure::ParameterError(e) => { @@ -777,9 +777,9 @@ impl OutboundPayments { } } - fn push_payment_path_failed_evs>>( - payment_id: PaymentId, payment_hash: PaymentHash, paths: Vec>, path_results: I, - pending_events: &Mutex> + fn push_path_failed_evs_and_scids>>( + payment_id: PaymentId, payment_hash: PaymentHash, route_params: &mut RouteParameters, + paths: Vec>, path_results: I, pending_events: &Mutex> ) { let mut events = pending_events.lock().unwrap(); debug_assert_eq!(paths.len(), path_results.len()); @@ -788,7 +788,9 @@ impl OutboundPayments { let failed_scid = if let APIError::InvalidRoute { .. } = e { None } else { - Some(path[0].short_channel_id) + let scid = path[0].short_channel_id; + route_params.payment_params.previously_failed_channels.push(scid); + Some(scid) }; events.push(events::Event::PaymentPathFailed { payment_id: Some(payment_id), diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 7c90f5601..1d6eba5ad 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -1857,13 +1857,15 @@ fn auto_retry_partial_failure() { payment_params: Some(route_params.payment_params.clone()), }; nodes[0].router.expect_find_route(route_params.clone(), Ok(send_route)); + let mut payment_params = route_params.payment_params.clone(); + payment_params.previously_failed_channels.push(chan_2_id); nodes[0].router.expect_find_route(RouteParameters { - payment_params: route_params.payment_params.clone(), - final_value_msat: amt_msat / 2, final_cltv_expiry_delta: TEST_FINAL_CLTV + payment_params, final_value_msat: amt_msat / 2, final_cltv_expiry_delta: TEST_FINAL_CLTV }, Ok(retry_1_route)); + let mut payment_params = route_params.payment_params.clone(); + payment_params.previously_failed_channels.push(chan_3_id); nodes[0].router.expect_find_route(RouteParameters { - payment_params: route_params.payment_params.clone(), - final_value_msat: amt_msat / 4, final_cltv_expiry_delta: TEST_FINAL_CLTV + payment_params, final_value_msat: amt_msat / 4, final_cltv_expiry_delta: TEST_FINAL_CLTV }, Ok(retry_2_route)); // Send a payment that will partially fail on send, then partially fail on retry, then succeed. @@ -2113,8 +2115,10 @@ fn retry_multi_path_single_failed_payment() { // On retry, split the payment across both channels. route.paths[0][0].fee_msat = 50_000_001; route.paths[1][0].fee_msat = 50_000_000; + let mut pay_params = route.payment_params.clone().unwrap(); + pay_params.previously_failed_channels.push(chans[1].short_channel_id.unwrap()); nodes[0].router.expect_find_route(RouteParameters { - payment_params: route.payment_params.clone().unwrap(), + payment_params: pay_params, // Note that the second request here requests the amount we originally failed to send, // not the amount remaining on the full payment, which should be changed. final_value_msat: 100_000_001, final_cltv_expiry_delta: TEST_FINAL_CLTV @@ -2196,9 +2200,11 @@ fn immediate_retry_on_failure() { route.paths[0][0].short_channel_id = chans[1].short_channel_id.unwrap(); route.paths[0][0].fee_msat = 50_000_000; route.paths[1][0].fee_msat = 50_000_001; + let mut pay_params = route_params.payment_params.clone(); + pay_params.previously_failed_channels.push(chans[0].short_channel_id.unwrap()); nodes[0].router.expect_find_route(RouteParameters { - payment_params: route_params.payment_params.clone(), - final_value_msat: amt_msat, final_cltv_expiry_delta: TEST_FINAL_CLTV + payment_params: pay_params, final_value_msat: amt_msat, + final_cltv_expiry_delta: TEST_FINAL_CLTV }, Ok(route.clone())); nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap(); -- 2.39.5