Lightning: Should waitinvoice and/or waitanyinvoice return something (an error?) on invoice expire?

[ZmnSCPxj]

When we waitinvoice an unpaid invoice, it stalls the json-rpc connection. But if the invoice expires, that connection remains stalled until another client process or connection deletes it.

Similarly if we decide waitinvoice should return on expiration, should (or should not) waitanyinvoice also return expired invoices (and allocate a pay_index also, or maybe call it completion_index instead)?

[rustyrussell]

waitinvoice should return once it's timed out, with an error that the invoice has timed out. That fits with the trivial API that it is.

waitanyinvoice should keep waiting for an invoice to be paid, ignoring timed out ones. That's what production systems will want: if you generate an invoice for every pageview, most will time out, and it doesn't matter.

@shesek you're our main JSON user so far, any opinions?

[ZmnSCPxj]

That's what production systems will want: if you generate an invoice for every pageview, most will time out, and it doesn't matter

I imagine most production systems will want to clear expired invoices from the database, though. Returning expired invoices in waitanyinvoice makes sense so the payment software can clean it from its database and then delinvoice it.

[shesek]

I'm currently only using waitanyinvoice (with an internal dispatcher) and not waitinvoice, but agree that waitinvoice should return an error when the invoice expires. I'm doing something similar in the long-polling HTTP API that provides similar functionality (though, only checking for expiry initially and not during the wait period -- which lightningd doesn't seem to do either).

As for waitanyinvoice: I don't have a strong opinion on that, but I think that it should ignore expired invoices rather than reporting them. I personally don't have a use for the expired ones and would just discard them. To clear expired invoices, I keep track of their expiry time (returned by lightningd when creating the invoice) on my own database and run a job to look up expired invoices every few hours. This way, its easier to ensure nothing slips through (due to downtime etc), and we don't really lose much for not getting them in real-time.

Perhaps it could be added as an optional include_expired flag for waitanyinvoice?

Edit:

though, only checking for expiry initially and not during the wait period

Not any more: https://github.com/ElementsProject/lightning-charge/commit/23cdf66bcc89288710810f663622b8d16fb21846

[ZmnSCPxj]

Perhaps it could be added as an optional include_expired flag for waitanyinvoice?

It would still require that pay_index be set for expired invoices still, and complicates our implementation of handling waiters on waitanyinvoice (we need to add a hook on invoice expiration and filter out waiters that have and do not have this flag set), so if there is no strong need for it, I would rather not implement reporting expired invoices in waitanyinvoice.

---

For waitinvoice, a simple thing would be to have a once-a-second timer that iterates over the invoices list and trigger with an error any expired invoices. But consider the case where there are millions of invoices, eek.

An alternative would be to keep track of the current lowest expiration for unpaid invoices, and set the timer to the lowest expiration. At initialization we set up the timer (or set it to UINT64_MAX if no unpaid unexpired invoices).

1. When the timer triggers, iterate over the invoice list (also keeping track of the next lowest expiration), and expire invoices that have achieved the expiration.
2. When a new invoice is created, check if its expiration is less than the current lowest expiration, and update the timer if so.
3. When an invoice is paid or deleted, do nothing! Even if the lowest-expiration invoices are paid/deleted, we would still need to iterate over the invoice list to find the next lowest expiration anyway. So keeping the lowest expiration (even if it is strictly not the lowest expiration anymore) is OK, that is when we update to the new lowest expiration (so that if multiple invoices are deleted or paid in short time, we do not reiterate over the list).

Or we can create a database index over the expiration times and get MIN() from that at each database update.

[ZmnSCPxj]

I believe @NicolasDorier was asking about waitanyinvoice before, maybe you can express an opinion regarding invoice expiration and waitanyinvoice?

[NicolasDorier]

@ZmnSCPxj I completely think waitanyinvoice should just ignore expired invoice.
If you want to cleanup your database, you can do it by polling the invoices time to time. (like once an hour)
The caller also knows before hand about the expiration, so you can also create a timer which check if the invoice is paid after the expiration and clean the database if not.

For waitinvoice, expiration should return something. But waitinvoice can't possibly work at scale. (Due to limitations on number of concurrent HTTP requests)

In BTCPay case, invoices are created even if the user does not pay with LN, so there will be tons of expirations.

[ZmnSCPxj]

Well, I think the general consensus is thus:

1. watinvoice should return an error on expiration.
2. waitanyinvoice should ignore expired invoices.

[ZmnSCPxj]

Okay, I studied the time system a little.

Our timers system uses timemono, however expiry is measured in timeabs.

So after measuring the lowest expiry, we need to subtract the time offset from the lowest expiry to current timeabs time from time_now, then use a timer_addrel with that offset.

[ZmnSCPxj]

I'll do this after #597 is either merged or closed. I intend to add a new state EXPIRED to enum invoice_status, which will get updated at timer triggering.