Use only the failed amount when retrying payments, not the full amt
authorMatt Corallo <git@bluematt.me>
Sat, 28 Jan 2023 02:14:26 +0000 (02:14 +0000)
committerMatt Corallo <git@bluematt.me>
Wed, 1 Feb 2023 21:16:18 +0000 (21:16 +0000)
When we landed the initial in-`ChannelManager` payment retries, we
stored the `RouteParameters` in the payment info, and then re-use
it as-is for new payments. `RouteParameters` is intended to store
the information specific to the *route*, `PaymentParameters` exists
to store information specific to a payment.

Worse, because we don't recalculate the amount stored in the
`RouteParameters` before fetching a new route with it, we end up
attempting to retry the full payment amount, rather than only the
failed part.

This issue brought to you by having redundant data in
datastructures, part 5,001.

lightning/src/ln/channelmanager.rs
lightning/src/ln/outbound_payment.rs
lightning/src/ln/payment_tests.rs

index 59d1dbfc93fe2d617107ed084de9d48883c4c657..e9b7fb94930c672f23317f3cb19ca3e9c3c37709 100644 (file)
@@ -7363,7 +7363,7 @@ where
                                                                        entry.insert(PendingOutboundPayment::Retryable {
                                                                                retry_strategy: None,
                                                                                attempts: PaymentAttempts::new(),
-                                                                               route_params: None,
+                                                                               payment_params: None,
                                                                                session_privs: [session_priv_bytes].iter().map(|a| *a).collect(),
                                                                                payment_hash: htlc.payment_hash,
                                                                                payment_secret,
index 08f600b2d8f46646edb5a90b952dfde6676bb287..f745363036b7b2caf4ba72ad8ab8bb01185b46e1 100644 (file)
@@ -16,6 +16,7 @@ use bitcoin::secp256k1::{self, Secp256k1, SecretKey};
 use crate::chain::keysinterface::{EntropySource, NodeSigner, Recipient};
 use crate::ln::{PaymentHash, PaymentPreimage, PaymentSecret};
 use crate::ln::channelmanager::{ChannelDetails, HTLCSource, IDEMPOTENCY_TIMEOUT_TICKS, MIN_HTLC_RELAY_HOLDING_CELL_MILLIS, PaymentId};
+use crate::ln::channelmanager::MIN_FINAL_CLTV_EXPIRY_DELTA as LDK_DEFAULT_MIN_FINAL_CLTV_EXPIRY_DELTA;
 use crate::ln::msgs::DecodeError;
 use crate::ln::onion_utils::HTLCFailReason;
 use crate::routing::router::{InFlightHtlcs, PaymentParameters, Route, RouteHop, RouteParameters, RoutePath, Router};
@@ -43,7 +44,7 @@ pub(crate) enum PendingOutboundPayment {
        Retryable {
                retry_strategy: Option<Retry>,
                attempts: PaymentAttempts,
-               route_params: Option<RouteParameters>,
+               payment_params: Option<PaymentParameters>,
                session_privs: HashSet<[u8; 32]>,
                payment_hash: PaymentHash,
                payment_secret: Option<PaymentSecret>,
@@ -102,9 +103,17 @@ impl PendingOutboundPayment {
                        _ => false,
                }
        }
+       fn payment_parameters(&mut self) -> Option<&mut PaymentParameters> {
+               match self {
+                       PendingOutboundPayment::Retryable { payment_params: Some(ref mut params), .. } => {
+                               Some(params)
+                       },
+                       _ => None,
+               }
+       }
        pub fn insert_previously_failed_scid(&mut self, scid: u64) {
-               if let PendingOutboundPayment::Retryable { route_params: Some(params), .. } = self {
-                       params.payment_params.previously_failed_channels.push(scid);
+               if let PendingOutboundPayment::Retryable { payment_params: Some(params), .. } = self {
+                       params.previously_failed_channels.push(scid);
                }
        }
        pub(super) fn is_fulfilled(&self) -> bool {
@@ -474,9 +483,18 @@ impl OutboundPayments {
                        let mut retry_id_route_params = None;
                        for (pmt_id, pmt) in outbounds.iter_mut() {
                                if pmt.is_auto_retryable_now() {
-                                       if let PendingOutboundPayment::Retryable { pending_amt_msat, total_msat, route_params: Some(params), .. } = pmt {
+                                       if let PendingOutboundPayment::Retryable { pending_amt_msat, total_msat, payment_params: Some(params), .. } = pmt {
                                                if pending_amt_msat < total_msat {
-                                                       retry_id_route_params = Some((*pmt_id, params.clone()));
+                                                       retry_id_route_params = Some((*pmt_id, RouteParameters {
+                                                               final_value_msat: *total_msat - *pending_amt_msat,
+                                                               final_cltv_expiry_delta:
+                                                                       if let Some(delta) = params.final_cltv_expiry_delta { delta }
+                                                                       else {
+                                                                               debug_assert!(false, "We always set the final_cltv_expiry_delta when a path fails");
+                                                                               LDK_DEFAULT_MIN_FINAL_CLTV_EXPIRY_DELTA.into()
+                                                                       },
+                                                               payment_params: params.clone(),
+                                                       }));
                                                        break
                                                }
                                        }
@@ -522,7 +540,7 @@ 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, Some(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.payment_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)
@@ -672,7 +690,7 @@ impl OutboundPayments {
 
        pub(super) fn add_new_pending_payment<ES: Deref>(
                &self, payment_hash: PaymentHash, payment_secret: Option<PaymentSecret>, payment_id: PaymentId,
-               route: &Route, retry_strategy: Option<Retry>, route_params: Option<RouteParameters>,
+               route: &Route, retry_strategy: Option<Retry>, payment_params: Option<PaymentParameters>,
                entropy_source: &ES, best_block_height: u32
        ) -> Result<Vec<[u8; 32]>, PaymentSendFailure> where ES::Target: EntropySource {
                let mut onion_session_privs = Vec::with_capacity(route.paths.len());
@@ -687,7 +705,7 @@ impl OutboundPayments {
                                let payment = entry.insert(PendingOutboundPayment::Retryable {
                                        retry_strategy,
                                        attempts: PaymentAttempts::new(),
-                                       route_params,
+                                       payment_params,
                                        session_privs: HashSet::new(),
                                        pending_amt_msat: 0,
                                        pending_fee_msat: Some(0),
@@ -965,6 +983,7 @@ impl OutboundPayments {
                let mut all_paths_failed = false;
                let mut full_failure_ev = None;
                let mut pending_retry_ev = None;
+               let mut retry = None;
                let attempts_remaining = if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(*payment_id) {
                        if !payment.get_mut().remove(&session_priv_bytes, Some(&path)) {
                                log_trace!(logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0));
@@ -978,6 +997,33 @@ impl OutboundPayments {
                        if let Some(scid) = short_channel_id {
                                payment.get_mut().insert_previously_failed_scid(scid);
                        }
+
+                       // We want to move towards only using the `PaymentParameters` in the outbound payments
+                       // map. However, for backwards-compatibility, we still need to support passing the
+                       // `PaymentParameters` data that was shoved in the HTLC (and given to us via
+                       // `payment_params`) back to the user.
+                       let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
+                       if let Some(params) = payment.get_mut().payment_parameters() {
+                               if params.final_cltv_expiry_delta.is_none() {
+                                       // This should be rare, but a user could provide None for the payment data, and
+                                       // we need it when we go to retry the payment, so fill it in.
+                                       params.final_cltv_expiry_delta = Some(path_last_hop.cltv_expiry_delta);
+                               }
+                               retry = Some(RouteParameters {
+                                       payment_params: params.clone(),
+                                       final_value_msat: path_last_hop.fee_msat,
+                                       final_cltv_expiry_delta: params.final_cltv_expiry_delta.unwrap(),
+                               });
+                       } else if let Some(params) = payment_params {
+                               retry = Some(RouteParameters {
+                                       payment_params: params.clone(),
+                                       final_value_msat: path_last_hop.fee_msat,
+                                       final_cltv_expiry_delta:
+                                               if let Some(delta) = params.final_cltv_expiry_delta { delta }
+                                               else { path_last_hop.cltv_expiry_delta },
+                               });
+                       }
+
                        if payment.get().remaining_parts() == 0 {
                                all_paths_failed = true;
                                if payment.get().abandoned() {
@@ -994,16 +1040,6 @@ impl OutboundPayments {
                        return
                };
                core::mem::drop(outbounds);
-               let mut retry = if let Some(payment_params_data) = payment_params {
-                       let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
-                       Some(RouteParameters {
-                               payment_params: payment_params_data.clone(),
-                               final_value_msat: path_last_hop.fee_msat,
-                               final_cltv_expiry_delta:
-                                       if let Some(delta) = payment_params_data.final_cltv_expiry_delta { delta }
-                                       else { path_last_hop.cltv_expiry_delta },
-                       })
-               } else { None };
                log_trace!(logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
 
                let path_failure = {
@@ -1115,13 +1151,13 @@ 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, None)),
+               (3, payment_params, option),
                (4, payment_secret, option),
-               (not_written, attempts, (static_value, PaymentAttempts::new())),
                (6, total_msat, required),
-               (not_written, route_params, (static_value, None)),
                (8, pending_amt_msat, required),
                (10, starting_block_height, required),
+               (not_written, retry_strategy, (static_value, None)),
+               (not_written, attempts, (static_value, PaymentAttempts::new())),
        },
        (3, Abandoned) => {
                (0, session_privs, required),
@@ -1212,7 +1248,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 }, Some(Retry::Attempts(1)), Some(route_params.clone()),
+                       &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(),
index b3acfc3209decee1f55c0d8c134b26944e61cd14..68074f1b59b33ecc8d47a88393004ed75b077734 100644 (file)
@@ -2417,3 +2417,161 @@ fn no_extra_retries_on_back_to_back_fail() {
                _ => panic!("Unexpected event"),
        }
 }
+
+#[test]
+fn test_simple_partial_retry() {
+       // In the first version of the in-`ChannelManager` payment retries, retries were sent for the
+       // full amount of the payment, rather than only the missing amount. Here we simply test for
+       // this by sending a payment with two parts, failing one, and retrying the second. Note that
+       // `TestRouter` will check that the `RouteParameters` (which contain the amount) matches the
+       // request.
+       let chanmon_cfgs = create_chanmon_cfgs(3);
+       let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
+       let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
+       let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
+
+       let chan_1_scid = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 10_000_000, 0).0.contents.short_channel_id;
+       let chan_2_scid = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 10_000_000, 0).0.contents.short_channel_id;
+
+       let amt_msat = 200_000_000;
+       let (_, payment_hash, _, payment_secret) = get_route_and_payment_hash!(&nodes[0], nodes[2], amt_msat);
+       #[cfg(feature = "std")]
+       let payment_expiry_secs = SystemTime::UNIX_EPOCH.elapsed().unwrap().as_secs() + 60 * 60;
+       #[cfg(not(feature = "std"))]
+       let payment_expiry_secs = 60 * 60;
+       let mut invoice_features = InvoiceFeatures::empty();
+       invoice_features.set_variable_length_onion_required();
+       invoice_features.set_payment_secret_required();
+       invoice_features.set_basic_mpp_optional();
+       let payment_params = PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), TEST_FINAL_CLTV)
+               .with_expiry_time(payment_expiry_secs as u64)
+               .with_features(invoice_features);
+       let route_params = RouteParameters {
+               payment_params,
+               final_value_msat: amt_msat,
+               final_cltv_expiry_delta: TEST_FINAL_CLTV,
+       };
+
+       let mut route = Route {
+               paths: vec![
+                       vec![RouteHop {
+                               pubkey: nodes[1].node.get_our_node_id(),
+                               node_features: nodes[1].node.node_features(),
+                               short_channel_id: chan_1_scid,
+                               channel_features: nodes[1].node.channel_features(),
+                               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(),
+                               node_features: nodes[2].node.node_features(),
+                               short_channel_id: chan_2_scid,
+                               channel_features: nodes[2].node.channel_features(),
+                               fee_msat: 100_000_000,
+                               cltv_expiry_delta: 100,
+                       }],
+                       vec![RouteHop {
+                               pubkey: nodes[1].node.get_our_node_id(),
+                               node_features: nodes[1].node.node_features(),
+                               short_channel_id: chan_1_scid,
+                               channel_features: nodes[1].node.channel_features(),
+                               fee_msat: 100_000,
+                               cltv_expiry_delta: 100,
+                       }, RouteHop {
+                               pubkey: nodes[2].node.get_our_node_id(),
+                               node_features: nodes[2].node.node_features(),
+                               short_channel_id: chan_2_scid,
+                               channel_features: nodes[2].node.channel_features(),
+                               fee_msat: 100_000_000,
+                               cltv_expiry_delta: 100,
+                       }]
+               ],
+               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()));
+       let mut second_payment_params = route_params.payment_params.clone();
+       second_payment_params.previously_failed_channels = vec![chan_2_scid];
+       // On retry, we'll only be asked for one path (or 100k sats)
+       route.paths.remove(0);
+       nodes[0].router.expect_find_route(RouteParameters {
+                       payment_params: second_payment_params,
+                       final_value_msat: amt_msat / 2, 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();
+       let htlc_updates = SendEvent::from_node(&nodes[0]);
+       check_added_monitors!(nodes[0], 1);
+       assert_eq!(htlc_updates.msgs.len(), 1);
+
+       nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &htlc_updates.msgs[0]);
+       nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &htlc_updates.commitment_msg);
+       check_added_monitors!(nodes[1], 1);
+       let (bs_first_raa, bs_first_cs) = get_revoke_commit_msgs!(nodes[1], nodes[0].node.get_our_node_id());
+
+       nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_first_raa);
+       check_added_monitors!(nodes[0], 1);
+       let second_htlc_updates = SendEvent::from_node(&nodes[0]);
+
+       nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_first_cs);
+       check_added_monitors!(nodes[0], 1);
+       let as_first_raa = get_event_msg!(nodes[0], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());
+
+       nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &second_htlc_updates.msgs[0]);
+       nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &second_htlc_updates.commitment_msg);
+       check_added_monitors!(nodes[1], 1);
+       let bs_second_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[0].node.get_our_node_id());
+
+       nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_first_raa);
+       check_added_monitors!(nodes[1], 1);
+       let bs_fail_update = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
+
+       nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_second_raa);
+       check_added_monitors!(nodes[0], 1);
+
+       nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &bs_fail_update.update_fail_htlcs[0]);
+       nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_fail_update.commitment_signed);
+       check_added_monitors!(nodes[0], 1);
+       let (as_second_raa, as_third_cs) = get_revoke_commit_msgs!(nodes[0], nodes[1].node.get_our_node_id());
+
+       nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_second_raa);
+       check_added_monitors!(nodes[1], 1);
+
+       nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_third_cs);
+       check_added_monitors!(nodes[1], 1);
+
+       let bs_third_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[0].node.get_our_node_id());
+
+       nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_third_raa);
+       check_added_monitors!(nodes[0], 1);
+
+       let mut events = nodes[0].node.get_and_clear_pending_events();
+       assert_eq!(events.len(), 2);
+       match events[0] {
+               Event::PaymentPathFailed { payment_hash: ev_payment_hash, payment_failed_permanently, ..  } => {
+                       assert_eq!(payment_hash, ev_payment_hash);
+                       assert_eq!(payment_failed_permanently, false);
+               },
+               _ => panic!("Unexpected event"),
+       }
+       match events[1] {
+               Event::PendingHTLCsForwardable { .. } => {},
+               _ => panic!("Unexpected event"),
+       }
+
+       nodes[0].node.process_pending_htlc_forwards();
+       let retry_htlc_updates = SendEvent::from_node(&nodes[0]);
+       check_added_monitors!(nodes[0], 1);
+
+       nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &retry_htlc_updates.msgs[0]);
+       commitment_signed_dance!(nodes[1], nodes[0], &retry_htlc_updates.commitment_msg, false, true);
+
+       expect_pending_htlcs_forwardable!(nodes[1]);
+       check_added_monitors!(nodes[1], 1);
+
+       let bs_forward_update = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
+       nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &bs_forward_update.update_add_htlcs[0]);
+       nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &bs_forward_update.update_add_htlcs[1]);
+       commitment_signed_dance!(nodes[2], nodes[1], &bs_forward_update.commitment_signed, false);
+
+       expect_pending_htlcs_forwardable!(nodes[2]);
+       expect_payment_claimable!(nodes[2], payment_hash, payment_secret, amt_msat);
+}