Skip to content

Expose pending payments through ChannelManager #1873

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 1 commit into from
Feb 3, 2023
Merged

Expose pending payments through ChannelManager #1873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
94 changes: 82 additions & 12 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1154,6 +1154,36 @@ impl ChannelDetails {
}
}

/// Used by [`ChannelManager::list_recent_payments`] to express the status of recent payments.
/// These include payments that have yet to find a successful path, or have unresolved HTLCs.
#[derive(Debug, PartialEq)]
pub enum RecentPaymentDetails {
/// When a payment is still being sent and awaiting successful delivery.
Pending {
/// Hash of the payment that is currently being sent but has yet to be fulfilled or
/// abandoned.
payment_hash: PaymentHash,
/// Total amount (in msat, excluding fees) across all paths for this payment,
/// not just the amount currently inflight.
total_msat: u64,
},
/// When a pending payment is fulfilled, we continue tracking it until all pending HTLCs have
/// been resolved. Upon receiving [`Event::PaymentSent`], we delay for a few minutes before the
/// payment is removed from tracking.
Fulfilled {
/// Hash of the payment that was claimed. `None` for serializations of [`ChannelManager`]
/// 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.
Abandoned {
/// Hash of the payment that we have given up trying to send.
payment_hash: PaymentHash,
},
}

/// Route hints used in constructing invoices for [phantom node payents].
///
/// [phantom node payments]: crate::chain::keysinterface::PhantomKeysManager
Expand Down Expand Up @@ -1691,6 +1721,34 @@ where
self.list_channels_with_filter(|&(_, ref channel)| channel.is_live())
}

/// Returns in an undefined order recent payments that -- if not fulfilled -- have yet to find a
/// successful path, or have unresolved HTLCs.
///
/// 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`]: events::Event::PaymentSent
pub fn list_recent_payments(&self) -> Vec<RecentPaymentDetails> {
self.pending_outbound_payments.pending_outbound_payments.lock().unwrap().iter()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the pending_outbound_payments field in OutboundPayments may be worth renaming to avoid self.pending_outbound_payments.pending_outbound_payments.

thoughts? cc // @jkczyz @valentinewallace @TheBlueMatt

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I would be supportive of reaming. Perhaps id_to_pending_payment, payment_by_id, or the like? Open to other alternatives given we may want to be deliberate about where "pending" is used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's leave it for another PR - there's a ton of code motion in channelmanager.rs and friends right now between lock changes, payment retries, etc. The smaller we can keep this PR the better.

.filter_map(|(_, pending_outbound_payment)| match pending_outbound_payment {
PendingOutboundPayment::Retryable { payment_hash, total_msat, .. } => {
Some(RecentPaymentDetails::Pending {
payment_hash: *payment_hash,
total_msat: *total_msat,
})
},
PendingOutboundPayment::Abandoned { payment_hash, .. } => {
Some(RecentPaymentDetails::Abandoned { payment_hash: *payment_hash })
},
PendingOutboundPayment::Fulfilled { payment_hash, .. } => {
Some(RecentPaymentDetails::Fulfilled { payment_hash: *payment_hash })
},
Comment on lines +1741 to +1746
Copy link
Contributor

Choose a reason for hiding this comment

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

@TheBlueMatt What's the use case for including these two? Looking back at #1157, seems the issue was originally filed to support the sample node. But a later comment noted using PendingOutboundPayment isn't sufficient: lightningdevkit/ldk-sample#40 (comment)

If we instead just want anything pending, shouldn't Retryable be enough? Do we care that there are still inflight HTLCs if we've either given up on the payment or already have the preimage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bump @TheBlueMatt

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops, sorry I missed this. I think we need to include the Fulfilled and Abandoned cases here as one use-case for listing pending payments is to figure out if you need to retry a payment - if you don't see a payment there, you should (consider) retrying it (as noted at #1157 (comment)). If the payment has been fulfilled, but you have not yet seen/processed the PaymentSent event, you may retry it, but shouldn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for clarifying @TheBlueMatt. I thought this was worth clarifying in docs too so I added it in d626653 for ChannelManager::list_pending_payments. Let me know if the wording can be improved/if I missed anything

PendingOutboundPayment::Legacy { .. } => None
})
.collect()
}

/// Helper function that issues the channel close events
fn issue_channel_close_events(&self, channel: &Channel<<SP::Target as SignerProvider>::Signer>, closure_reason: ClosureReason) {
let mut pending_events_lock = self.pending_events.lock().unwrap();
Expand Down Expand Up @@ -2415,9 +2473,14 @@ where

/// Sends a payment along a given route.
///
/// Value parameters are provided via the last hop in route, see documentation for RouteHop
/// Value parameters are provided via the last hop in route, see documentation for [`RouteHop`]
/// fields for more info.
///
/// May generate SendHTLCs message(s) event on success, which should be relayed (e.g. via
/// [`PeerManager::process_events`]).
///
/// # Avoiding Duplicate Payments
///
/// 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
Expand All @@ -2430,12 +2493,16 @@ where
/// consider using the [`PaymentHash`] as the key for tracking payments. In that case, the
/// [`PaymentId`] should be a copy of the [`PaymentHash`] bytes.
///
/// May generate SendHTLCs message(s) event on success, which should be relayed (e.g. via
/// [`PeerManager::process_events`]).
/// Additionally, in the scenario where we begin the process of sending a payment, but crash
/// before `send_payment` returns (or prior to [`ChannelMonitorUpdate`] persistence if you're
/// using [`ChannelMonitorUpdateStatus::InProgress`]), the payment may be lost on restart. See
/// [`ChannelManager::list_recent_payments`] for more information.
///
/// # Possible Error States on [`PaymentSendFailure`]
///
/// Each path may have a different return value, and PaymentSendValue may return a Vec with
/// each entry matching the corresponding-index entry in the route paths, see
/// PaymentSendFailure for more info.
/// [`PaymentSendFailure`] for more info.
///
/// In general, a path may raise:
/// * [`APIError::InvalidRoute`] when an invalid route or forwarding parameter (cltv_delta, fee,
Expand All @@ -2450,18 +2517,21 @@ where
/// irrevocably committed to on our end. In such a case, do NOT retry the payment with a
/// different route unless you intend to pay twice!
///
/// payment_secret is unrelated to payment_hash (or PaymentPreimage) and exists to authenticate
/// the sender to the recipient and prevent payment-probing (deanonymization) attacks. For
/// newer nodes, it will be provided to you in the invoice. If you do not have one, the Route
/// must not contain multiple paths as multi-path payments require a recipient-provided
/// payment_secret.
/// # A caution on `payment_secret`
///
/// `payment_secret` is unrelated to `payment_hash` (or [`PaymentPreimage`]) and exists to
/// authenticate the sender to the recipient and prevent payment-probing (deanonymization)
/// attacks. For newer nodes, it will be provided to you in the invoice. If you do not have one,
/// the [`Route`] must not contain multiple paths as multi-path payments require a
/// recipient-provided `payment_secret`.
///
/// If a payment_secret *is* provided, we assume that the invoice had the payment_secret feature
/// bit set (either as required or as available). If multiple paths are present in the Route,
/// we assume the invoice had the basic_mpp feature set.
/// If a `payment_secret` *is* provided, we assume that the invoice had the payment_secret
/// feature bit set (either as required or as available). If multiple paths are present in the
/// [`Route`], we assume the invoice had the basic_mpp feature set.
///
/// [`Event::PaymentSent`]: events::Event::PaymentSent
/// [`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> {
let best_block_height = self.best_block.read().unwrap().height();
self.pending_outbound_payments
Expand Down
30 changes: 27 additions & 3 deletions lightning/src/ln/payment_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use crate::chain::channelmonitor::{ANTI_REORG_DELAY, LATENCY_GRACE_PERIOD_BLOCKS
use crate::chain::keysinterface::EntropySource;
use crate::chain::transaction::OutPoint;
use crate::ln::channel::EXPIRE_PREV_CONFIG_TICKS;
use crate::ln::channelmanager::{BREAKDOWN_TIMEOUT, ChannelManager, MPP_TIMEOUT_TICKS, MIN_CLTV_EXPIRY_DELTA, PaymentId, PaymentSendFailure, IDEMPOTENCY_TIMEOUT_TICKS};
use crate::ln::channelmanager::{BREAKDOWN_TIMEOUT, ChannelManager, MPP_TIMEOUT_TICKS, MIN_CLTV_EXPIRY_DELTA, PaymentId, PaymentSendFailure, IDEMPOTENCY_TIMEOUT_TICKS, RecentPaymentDetails};
use crate::ln::features::InvoiceFeatures;
use crate::ln::msgs;
use crate::ln::msgs::ChannelMessageHandler;
Expand Down Expand Up @@ -1274,7 +1274,11 @@ fn test_trivial_inflight_htlc_tracking(){
let (_, _, chan_2_id, _) = create_announced_chan_between_nodes(&nodes, 1, 2);

// Send and claim the payment. Inflight HTLCs should be empty.
send_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 500000);
let (route, payment_hash, payment_preimage, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], 500000);
nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret), PaymentId(payment_hash.0)).unwrap();
check_added_monitors!(nodes[0], 1);
pass_along_route(&nodes[0], &[&vec!(&nodes[1], &nodes[2])[..]], 500000, payment_hash, payment_secret);
claim_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], payment_preimage);
{
let inflight_htlcs = node_chanmgrs[0].compute_inflight_htlcs();

Expand All @@ -1299,9 +1303,17 @@ fn test_trivial_inflight_htlc_tracking(){
assert_eq!(chan_1_used_liquidity, None);
assert_eq!(chan_2_used_liquidity, None);
}
let pending_payments = nodes[0].node.list_recent_payments();
assert_eq!(pending_payments.len(), 1);
assert_eq!(pending_payments[0], RecentPaymentDetails::Fulfilled { payment_hash: Some(payment_hash) });

// Remove fulfilled payment
for _ in 0..=IDEMPOTENCY_TIMEOUT_TICKS {
nodes[0].node.timer_tick_occurred();
}

// Send the payment, but do not claim it. Our inflight HTLCs should contain the pending payment.
let (payment_preimage, _, _) = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 500000);
let (payment_preimage, payment_hash, _) = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 500000);
{
let inflight_htlcs = node_chanmgrs[0].compute_inflight_htlcs();

Expand All @@ -1327,9 +1339,18 @@ fn test_trivial_inflight_htlc_tracking(){
assert_eq!(chan_1_used_liquidity, Some(501000));
assert_eq!(chan_2_used_liquidity, Some(500000));
}
let pending_payments = nodes[0].node.list_recent_payments();
assert_eq!(pending_payments.len(), 1);
assert_eq!(pending_payments[0], RecentPaymentDetails::Pending { payment_hash, total_msat: 500000 });

// Now, let's claim the payment. This should result in the used liquidity to return `None`.
claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage);

// Remove fulfilled payment
for _ in 0..=IDEMPOTENCY_TIMEOUT_TICKS {
nodes[0].node.timer_tick_occurred();
}

{
let inflight_htlcs = node_chanmgrs[0].compute_inflight_htlcs();

Expand All @@ -1354,6 +1375,9 @@ fn test_trivial_inflight_htlc_tracking(){
assert_eq!(chan_1_used_liquidity, None);
assert_eq!(chan_2_used_liquidity, None);
}

let pending_payments = nodes[0].node.list_recent_payments();
assert_eq!(pending_payments.len(), 0);
}

#[test]
Expand Down