Skip to content

Abandon payments on behalf of the user and remove manual retries #2008

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Feb 16, 2023
Merged

Abandon payments on behalf of the user and remove manual retries #2008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8d0c08c
Fix indentation in outbound payment mark_abandoned
valentinewallace Feb 3, 2023
f871210
Pass pending events to outbound_payments::abandon_payment
valentinewallace Feb 3, 2023
07dd8b2
Abandon payment if retry fails in process_pending_htlcs
valentinewallace Feb 3, 2023
82e0880
Abandon payment on behalf of the user on payment path failure
valentinewallace Feb 3, 2023
13e60da
When processing pending htlcs, abandon outbounds that are not retryable
valentinewallace Feb 4, 2023
5c6d8a7
Remove retry_payments method
valentinewallace Feb 5, 2023
da34ada
Reword and fix grammar in PartialFailure docs
valentinewallace Feb 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions fuzz/src/chanmon_consistency.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -914,6 +914,7 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {
events::Event::PaymentClaimed { .. } => {},
events::Event::PaymentPathSuccessful { .. } => {},
events::Event::PaymentPathFailed { .. } => {},
events::Event::PaymentFailed { .. } => {},
events::Event::ProbeSuccessful { .. } | events::Event::ProbeFailed { .. } => {
// Even though we don't explicitly send probes, because probes are
// detected based on hashing the payment hash+preimage, its rather
Expand Down
8 changes: 7 additions & 1 deletion lightning/src/ln/chanmon_update_fail_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1752,12 +1752,18 @@ fn test_monitor_update_on_pending_forwards() {
commitment_signed_dance!(nodes[0], nodes[1], bs_updates.commitment_signed, false, true);

let events = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events.len(), 2);
assert_eq!(events.len(), 3);
if let Event::PaymentPathFailed { payment_hash, payment_failed_permanently, .. } = events[0] {
assert_eq!(payment_hash, payment_hash_1);
assert!(payment_failed_permanently);
} else { panic!("Unexpected event!"); }
match events[1] {
Event::PaymentFailed { payment_hash, .. } => {
assert_eq!(payment_hash, payment_hash_1);
},
_ => panic!("Unexpected event"),
}
match events[2] {
Event::PendingHTLCsForwardable { .. } => { },
_ => panic!("Unexpected event"),
};
Expand Down
53 changes: 16 additions & 37 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1190,9 +1190,9 @@ pub enum RecentPaymentDetails {
/// made before LDK version 0.0.104.
payment_hash: Option<PaymentHash>,
},
/// After a payment is explicitly abandoned by calling [`ChannelManager::abandon_payment`], it
/// is marked as abandoned until an [`Event::PaymentFailed`] is generated. A payment could also
/// be marked as abandoned if pathfinding fails repeatedly or retries have been exhausted.
/// After a payment's retries are exhausted per the provided [`Retry`], or it is explicitly
/// abandoned via [`ChannelManager::abandon_payment`], it is marked as abandoned until all
/// pending HTLCs for this payment resolve and an [`Event::PaymentFailed`] is generated.
Abandoned {
/// Hash of the payment that we have given up trying to send.
payment_hash: PaymentHash,
Expand Down Expand Up @@ -1718,7 +1718,7 @@ where
///
/// This can be useful for payments that may have been prepared, but ultimately not sent, as a
/// result of a crash. If such a payment exists, is not listed here, and an
/// [`Event::PaymentSent`] has not been received, you may consider retrying the payment.
/// [`Event::PaymentSent`] has not been received, you may consider resending the payment.
///
/// [`Event::PaymentSent`]: events::Event::PaymentSent
pub fn list_recent_payments(&self) -> Vec<RecentPaymentDetails> {
Expand Down Expand Up @@ -2475,8 +2475,8 @@ where
/// If a pending payment is currently in-flight with the same [`PaymentId`] provided, this
/// method will error with an [`APIError::InvalidRoute`]. 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`].
/// [`Event::PaymentSent`] or [`Event::PaymentFailed`]) LDK will not stop you from sending a
/// second payment with the same [`PaymentId`].
///
/// 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
Expand Down Expand Up @@ -2521,6 +2521,7 @@ where
/// [`Route`], we assume the invoice had the basic_mpp feature set.
///
/// [`Event::PaymentSent`]: events::Event::PaymentSent
/// [`Event::PaymentFailed`]: events::Event::PaymentFailed
/// [`PeerManager::process_events`]: crate::ln::peer_handler::PeerManager::process_events
/// [`ChannelMonitorUpdateStatus::InProgress`]: crate::chain::ChannelMonitorUpdateStatus::InProgress
pub fn send_payment(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>, payment_id: PaymentId) -> Result<(), PaymentSendFailure> {
Expand Down Expand Up @@ -2558,48 +2559,25 @@ where
}


/// Retries a payment along the given [`Route`].
/// Signals that no further retries for the given payment should occur. Useful if you have a
/// pending outbound payment with retries remaining, but wish to stop retrying the payment before
/// retries are exhausted.
///
/// Errors returned are a superset of those returned from [`send_payment`], so see
/// [`send_payment`] documentation for more details on errors. This method will also error if the
/// retry amount puts the payment more than 10% over the payment's total amount, if the payment
/// for the given `payment_id` cannot be found (likely due to timeout or success), or if
/// further retries have been disabled with [`abandon_payment`].
///
/// [`send_payment`]: [`ChannelManager::send_payment`]
/// [`abandon_payment`]: [`ChannelManager::abandon_payment`]
pub fn retry_payment(&self, route: &Route, payment_id: PaymentId) -> Result<(), PaymentSendFailure> {
let best_block_height = self.best_block.read().unwrap().height();
self.pending_outbound_payments.retry_payment_with_route(route, payment_id, &self.entropy_source, &self.node_signer, best_block_height,
|path, payment_params, payment_hash, payment_secret, total_value, cur_height, payment_id, keysend_preimage, session_priv|
self.send_payment_along_path(path, payment_params, payment_hash, payment_secret, total_value, cur_height, payment_id, keysend_preimage, session_priv))
}

/// Signals that no further retries for the given payment will occur.
///
/// After this method returns, no future calls to [`retry_payment`] for the given `payment_id`
/// are allowed. If no [`Event::PaymentFailed`] event had been generated before, one will be
/// generated as soon as there are no remaining pending HTLCs for this payment.
/// If no [`Event::PaymentFailed`] event had been generated before, one will be generated as soon
/// as there are no remaining pending HTLCs for this payment.
///
/// Note that calling this method does *not* prevent a payment from succeeding. You must still
/// wait until you receive either a [`Event::PaymentFailed`] or [`Event::PaymentSent`] event to
/// determine the ultimate status of a payment.
///
/// If an [`Event::PaymentFailed`] event is generated and we restart without this
/// [`ChannelManager`] having been persisted, the payment may still be in the pending state
/// upon restart. This allows further calls to [`retry_payment`] (and requiring a second call
/// to [`abandon_payment`] to mark the payment as failed again). Otherwise, future calls to
/// [`retry_payment`] will fail with [`PaymentSendFailure::ParameterError`].
/// [`ChannelManager`] having been persisted, another [`Event::PaymentFailed`] may be generated.
///
/// [`abandon_payment`]: Self::abandon_payment
/// [`retry_payment`]: Self::retry_payment
/// [`Event::PaymentFailed`]: events::Event::PaymentFailed
/// [`Event::PaymentSent`]: events::Event::PaymentSent
pub fn abandon_payment(&self, payment_id: PaymentId) {
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);
if let Some(payment_failed_ev) = self.pending_outbound_payments.abandon_payment(payment_id) {
self.pending_events.lock().unwrap().push(payment_failed_ev);
}
self.pending_outbound_payments.abandon_payment(payment_id, &self.pending_events);
}

/// Send a spontaneous payment, which is a payment that does not require the recipient to have
Expand Down Expand Up @@ -3372,7 +3350,8 @@ where

let best_block_height = self.best_block.read().unwrap().height();
self.pending_outbound_payments.check_retry_payments(&self.router, || self.list_usable_channels(),
|| self.compute_inflight_htlcs(), &self.entropy_source, &self.node_signer, best_block_height, &self.logger,
|| self.compute_inflight_htlcs(), &self.entropy_source, &self.node_signer, best_block_height,
&self.pending_events, &self.logger,
|path, payment_params, payment_hash, payment_secret, total_value, cur_height, payment_id, keysend_preimage, session_priv|
self.send_payment_along_path(path, payment_params, payment_hash, payment_secret, total_value, cur_height, payment_id, keysend_preimage, session_priv));

Expand Down
57 changes: 22 additions & 35 deletions lightning/src/ln/functional_test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1800,17 +1800,18 @@ macro_rules! expect_payment_failed {
}

pub fn expect_payment_failed_conditions_event<'a, 'b, 'c, 'd, 'e>(
node: &'a Node<'b, 'c, 'd>, payment_failed_event: Event, expected_payment_hash: PaymentHash,
payment_failed_events: Vec<Event>, expected_payment_hash: PaymentHash,
expected_payment_failed_permanently: bool, conditions: PaymentFailedConditions<'e>
) {
let expected_payment_id = match payment_failed_event {
if conditions.expected_mpp_parts_remain { assert_eq!(payment_failed_events.len(), 1); } else { assert_eq!(payment_failed_events.len(), 2); }
let expected_payment_id = match &payment_failed_events[0] {
Event::PaymentPathFailed { payment_hash, payment_failed_permanently, path, retry, payment_id, network_update, short_channel_id,
#[cfg(test)]
error_code,
#[cfg(test)]
error_data, .. } => {
assert_eq!(payment_hash, expected_payment_hash, "unexpected payment_hash");
assert_eq!(payment_failed_permanently, expected_payment_failed_permanently, "unexpected payment_failed_permanently value");
assert_eq!(*payment_hash, expected_payment_hash, "unexpected payment_hash");
assert_eq!(*payment_failed_permanently, expected_payment_failed_permanently, "unexpected payment_failed_permanently value");
assert!(retry.is_some(), "expected retry.is_some()");
assert_eq!(retry.as_ref().unwrap().final_value_msat, path.last().unwrap().fee_msat, "Retry amount should match last hop in path");
assert_eq!(retry.as_ref().unwrap().payment_params.payee_pubkey, path.last().unwrap().pubkey, "Retry payee node_id should match last hop in path");
Expand Down Expand Up @@ -1839,7 +1840,7 @@ pub fn expect_payment_failed_conditions_event<'a, 'b, 'c, 'd, 'e>(
},
Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent }) if chan_closed => {
if let Some(scid) = conditions.expected_blamed_scid {
assert_eq!(short_channel_id, scid);
assert_eq!(*short_channel_id, scid);
}
assert!(is_permanent);
},
Expand All @@ -1853,10 +1854,7 @@ pub fn expect_payment_failed_conditions_event<'a, 'b, 'c, 'd, 'e>(
_ => panic!("Unexpected event"),
};
if !conditions.expected_mpp_parts_remain {
node.node.abandon_payment(expected_payment_id);
let events = node.node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
match events[0] {
match &payment_failed_events[1] {
Event::PaymentFailed { ref payment_hash, ref payment_id } => {
assert_eq!(*payment_hash, expected_payment_hash, "unexpected second payment_hash");
assert_eq!(*payment_id, expected_payment_id);
Expand All @@ -1870,9 +1868,8 @@ pub fn expect_payment_failed_conditions<'a, 'b, 'c, 'd, 'e>(
node: &'a Node<'b, 'c, 'd>, expected_payment_hash: PaymentHash, expected_payment_failed_permanently: bool,
conditions: PaymentFailedConditions<'e>
) {
let mut events = node.node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
expect_payment_failed_conditions_event(node, events.pop().unwrap(), expected_payment_hash, expected_payment_failed_permanently, conditions);
let events = node.node.get_and_clear_pending_events();
expect_payment_failed_conditions_event(events, expected_payment_hash, expected_payment_failed_permanently, conditions);
}

pub fn send_along_route_with_secret<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, route: Route, expected_paths: &[&[&Node<'a, 'b, 'c>]], recv_value: u64, our_payment_hash: PaymentHash, our_payment_secret: PaymentSecret) -> PaymentId {
Expand Down Expand Up @@ -2157,22 +2154,6 @@ pub fn fail_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expe
}

pub fn pass_failed_payment_back<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_paths_slice: &[&[&Node<'a, 'b, 'c>]], skip_last: bool, our_payment_hash: PaymentHash) {
let expected_payment_id = pass_failed_payment_back_no_abandon(origin_node, expected_paths_slice, skip_last, our_payment_hash);
if !skip_last {
origin_node.node.abandon_payment(expected_payment_id.unwrap());
let events = origin_node.node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
match events[0] {
Event::PaymentFailed { ref payment_hash, ref payment_id } => {
assert_eq!(*payment_hash, our_payment_hash, "unexpected second payment_hash");
assert_eq!(*payment_id, expected_payment_id.unwrap());
}
_ => panic!("Unexpected second event"),
}
}
}

pub fn pass_failed_payment_back_no_abandon<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_paths_slice: &[&[&Node<'a, 'b, 'c>]], skip_last: bool, our_payment_hash: PaymentHash) -> Option<PaymentId> {
let mut expected_paths: Vec<_> = expected_paths_slice.iter().collect();
check_added_monitors!(expected_paths[0].last().unwrap(), expected_paths.len());

Expand All @@ -2196,8 +2177,6 @@ pub fn pass_failed_payment_back_no_abandon<'a, 'b, 'c>(origin_node: &Node<'a, 'b
per_path_msgs.sort_unstable_by(|(_, node_id_a), (_, node_id_b)| node_id_a.cmp(node_id_b));
expected_paths.sort_unstable_by(|path_a, path_b| path_a[path_a.len() - 2].node.get_our_node_id().cmp(&path_b[path_b.len() - 2].node.get_our_node_id()));

let mut expected_payment_id = None;

for (i, (expected_route, (path_msgs, next_hop))) in expected_paths.iter().zip(per_path_msgs.drain(..)).enumerate() {
let mut next_msgs = Some(path_msgs);
let mut expected_next_node = next_hop;
Expand Down Expand Up @@ -2245,8 +2224,9 @@ pub fn pass_failed_payment_back_no_abandon<'a, 'b, 'c>(origin_node: &Node<'a, 'b
assert!(origin_node.node.get_and_clear_pending_msg_events().is_empty());
commitment_signed_dance!(origin_node, prev_node, next_msgs.as_ref().unwrap().1, false);
let events = origin_node.node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
expected_payment_id = Some(match events[0] {
if i == expected_paths.len() - 1 { assert_eq!(events.len(), 2); } else { assert_eq!(events.len(), 1); }

let expected_payment_id = match events[0] {
Event::PaymentPathFailed { payment_hash, payment_failed_permanently, all_paths_failed, ref path, ref payment_id, .. } => {
assert_eq!(payment_hash, our_payment_hash);
assert!(payment_failed_permanently);
Expand All @@ -2257,7 +2237,16 @@ pub fn pass_failed_payment_back_no_abandon<'a, 'b, 'c>(origin_node: &Node<'a, 'b
payment_id.unwrap()
},
_ => panic!("Unexpected event"),
});
};
if i == expected_paths.len() - 1 {
match events[1] {
Event::PaymentFailed { ref payment_hash, ref payment_id } => {
assert_eq!(*payment_hash, our_payment_hash, "unexpected second payment_hash");
assert_eq!(*payment_id, expected_payment_id);
}
_ => panic!("Unexpected second event"),
}
}
Comment on lines +2241 to +2249
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I completely understand the changes to the testing behavior. Not necessarily here -- which may be justified -- but elsewhere where the number of events has increased because of the additional Event::PaymentFailed generated. Wouldn't it be less invasive if the Event::PaymentFailed case was only checked in a dedicated test? Then when testing other behavior, we could have retries remaining such that no new event are generated so those tests are more narrowly focused.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A portion of the changed tests won't have retryable HTLCs, so the PaymentFailed will be generated no matter what. Most of the others that I'm seeing tend to use the test utils such as route_payment, but when I update that util to use _with_retry I get 55 test failures.. I think it might be tricky since having retryable failed HTLCs will often lead to the generation of retry msg events to be handled. That said, it would probably be good in the long term to move the test utils to use _with_retry

}
}

Expand All @@ -2266,8 +2255,6 @@ pub fn pass_failed_payment_back_no_abandon<'a, 'b, 'c>(origin_node: &Node<'a, 'b
assert!(expected_paths[0].last().unwrap().node.get_and_clear_pending_events().is_empty());
assert!(expected_paths[0].last().unwrap().node.get_and_clear_pending_msg_events().is_empty());
check_added_monitors!(expected_paths[0].last().unwrap(), 0);

expected_payment_id
}

pub fn fail_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_path: &[&Node<'a, 'b, 'c>], our_payment_hash: PaymentHash) {
Expand Down
Loading