Expose pending payment set from ChannelManager

[TheBlueMatt]

Users probably need that, eg sample, see lightningdevkit/ldk-sample#40 (comment)

[TheBlueMatt]

Note that this is pretty important in part because race conditions on the users' end may lead to it being unclear if a payment is really in-flight - if a user goes to call send_payment they have to assume it is now in-flight, even if they crashed before send_payment returned or was able to do anything at all.

The API here for this may be more complicated than just shoving them into ChannelDetails as #1274 suggested, however, it needs to be focused on payments as suggested in this issue instead - HTLCs could be added again later after some event that's sitting in a queue gets processed, if we first require the user call abandon_payment and then check if the payment is still pending, then we know the payment won't be retried. Note that this does not mean that the payment failed, it may very well have a PaymentSuccess event pending in a queue. For that, we'll need some kind of queue-drain function on lightning-background-processor, which users will have to call before checking if a payment is failed.

[jurvis]

@TheBlueMatt I'm trying to work on this issue alongside moving HTLC-tracking to ChannelManager as per (2) in this comment: #1643 (comment)

I noticed we can derive most information about inflight payments by looking into Channels that ChannelManager has since we currently use HTLCSources from it to send PaymentPathSuccessful/Failure events to InvoicePayer.

I have a few questions that came out of my investigation so far:

• In the Channel struct, it appears that we track HTLCSources in various places, notably in monitor_pending_failures and monitor_pending_finalized_fulfills -- can we treat them as disjoint sets? If so, is their union representative of all the inflight HTLCs at any moment for a channel?
• Do we consider HTLCs in holding cells as inflight?

Additionally, at a higher level: what is the current thinking around (1) an ideal API for exposing pending payments to users and (2) how to track inflight HTLCs in ChannelManager?

I believe the @jkczyz mentioned at some point for (2) to consider either creating a new trait or adding InflightHtlcs to Payer -- but I was wondering if you have any separate thoughts there.

[jkczyz]

I believe the @jkczyz mentioned at some point for (2) to consider either creating a new trait or adding InflightHtlcs to Payer -- but I was wondering if you have any separate thoughts there.

One way or another, the pending HTLCs will need to be accessed through Payer. I think the question is should ChannelManager have it's own interface such that impl Payer for ChannelManager needs to convert from that to InflightHtlcs. Or if it should directly use InflightHtlcs.

[TheBlueMatt]

In the Channel struct, it appears that we track HTLCSources in various places, notably in monitor_pending_failures and monitor_pending_finalized_fulfills -- can we treat them as disjoint sets? If so, is their union representative of all the inflight HTLCs at any moment for a channel?

These are waiting to be failed back, they are no longer in-flight and should be ignored.

Do we consider HTLCs in holding cells as inflight?

If we're waiting to add an HTLC, yes, we should count that.

I think the question is should ChannelManager have it's own interface such that impl Payer for ChannelManager needs to convert from that to InflightHtlcs. Or if it should directly use InflightHtlcs.

ISTM we could just use InFlightHtlcs - my big concern here is ensuring that we don't communicate to users that this interface should be used to query for pending payments but only for scoring purposes. Using InFlightHtlcs or something that really just implements score-wrapping communicates that well.

[jkczyz]

ISTM we could just use InFlightHtlcs - my big concern here is ensuring that we don't communicate to users that this interface should be used to query for pending payments but only for scoring purposes. Using InFlightHtlcs or something that really just implements score-wrapping communicates that well.

That means while using InFlightHtlcs in ChannelManager solves the need of tracking these in InvoicePayer, it doesn't solve the issue in discussion here. Are you saying we should have independent solutions?

[TheBlueMatt]

Yes, I think we'll want independent solutions. If nothing else this API is going to want to include trampoline and forwarded HTLCs (and be HTLC-focused) whereas the pending-payments querying logic should communicate payment status (and maybe not even expose anything at the HTLC level yet, given the race concerns I highlighted previously). If we do the PaymentId-based idempotency stuff then I think we can feel more comfortable with exposing payment level data in a way that isn't intently race-y.

[TheBlueMatt]

Note that that won't solve the "what's my current balance" question, but that likely needs a third solution (or could go in the pending payment access) because it needs to consider which HTLCs are part of a forward and which are outbound (and look at the expected payment amount for outbound payments, not the current pending-HTLC amount).

[jurvis]

@TheBlueMatt I'm going to start looking to exposing pending payment information after landing #1830, but had a couple of questions to get us going:

1. Is it sufficient to map through ChannelManager's pending_outbound_payments for PendingOutboundPayments of the Retryable variant and expose its fields?
2. What information should we expose to users, and how should the API ideally work?

@jkczyz and I also spoke offline about whether or not we should try and order the pending payments, but it looks like the only information we know about them is based on starting_block_height, so payments made within a block time are likely going to be "unordered".

My initial intuition is that it is perhaps better to just say outright that the payments are unordered, leaving the user to figure out how to track their state by PaymentId, rather than imply a false expectation of ordering. Any thoughts?

[TheBlueMatt]

Yea, so I think we want to just do it based on the pending map, indeed. I dont think we want to expose the amount currently in-flight, but rather only the total and the status (retryable/fulfilled/abandoned).

[jurvis]

seeking concept ACK in PR above. let me know if I'm misunderstanding anything

[TheBlueMatt]

Fixed in #1873.