Add an option to subscribe to every Hold Invoice at once

sergioabril commented 5 years ago

Background

There is no easy way to get Hold Invoice events with one rpc subscription. You have to track them individually, and whenever you start tracking a bunch of them, it doesn't make sense keeping so many rpc subscriptions open, when you could just subscribe once and get all the events there (Just like it happens with regular invoices). This could be a problem for any service that could want to provide Hold Invoices as a solution for their users, and needed to track lots of them at once.

Your environment

Lnd 0.6.1
(Any environment or setup)

Steps to reproduce

Subscribe to subscribeInvoices
You will start getting standard events for invoices (even for hold invoices), such as OPEN and SETTLE, but not those advanced events used on Hold specific ones, such as ACCEPT or CANCEL.
You could then try subscribing to SubscribeSingleInvoice using the Sub-Server Invoices, but you should do that for each of the Hold invoices you are tracking.

Problem

Whenever you are tracking a bunch of hold invoices, doesn't make sense to subscribe to each of them individually.

Possible solutions

One option could be including an advanced SubscribeInvoices on the invoicesrpc Sub-Server, that gets every STATE change for every invoice. I'm not sure if it's possible to filter it to get Hold-Invoices-only here, but if it's not, then could be used as an advanced replacement of the regular SubscribeInvoices we already have.
Other option might be including Hold Invoice events on the regular subscribeInvoices rpc call. To avoid the hassle of receiving too many updates, it could be an optional parameter whenever you subscribe, set to false by default.

I'm not sure how this works under the hood, but I guess that maybe you would be getting all the state events for every invoice, not just for hold ones, and that's why you haven't included it as an option yet. If that's the case, and is not possible to filter out invoices by its kind before firing the events, then at least would be great to have a "get all the states for every invoice" as an advanced option (whether option 1 or option 2)

Of course, would be great to have a subscribeHoldInvoices on the sub-server invoices, that gets every STATE event, but for hold invoices only

Roasbeef commented 5 years ago

Can't this be emulated with the existing calls by making individual subscriptions for each open invoice you know of? Depending on your language, you can likely combine all those streams into one (say using something like a python generator that takes other generators and yields them all).

sergioabril commented 5 years ago

I guess they could probably be combined, and for a small amount of them it would be probably fine (using Node.js). However, if you have a few hundred open hold invoices, I can imagine things would get messy; specially if you lose connection at any point and have to reconnect every stream for every (still) open invoice; I mean, it's doable, but much easier with a single subscription, just like happens with regular Invoices.

I also understand it is not a common case for regular wallets, but will probably be for any LN service using Hold invoices (That's why it could maybe be added to the Sub Server invoices as an advanced version, leaving the regular subscribeInvoices as it is)

On 27 May 2019, at 21:57, Olaoluwa Osuntokun notifications@github.com wrote:

Can't this be emulated with the existing calls by making individual subscriptions for each open invoice you know of? Depending on your language, you can likely combine all those streams into one (say using something like a python generator that takes other generators and yields them all).

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

joostjager commented 4 years ago

I think that a variation on Subscribe(All)Invoices that sends out updates for every event is the way to go. Not only are the Accepted and Canceled events currently not sent, but the stream also does not include updates to the invoice htlc list that don't necessarily change the invoice state. Especially for multi-part receives, it may be useful to get some insight in how the total set is coming together.

One question is how to signal to SubscribeInvoices what the starting point is from which backlogged events need to be sent. Currently there is add_index and settle_index, but with the increased state space it may be better to introduce something new. Either an always increasing global update counter (goes up for every commit to the invoice db) or alternatively a timestamp. Most data in the invoice db already includes a timestamp, so that could possibly be implemented without a db migration. A timestamp also has more meaning than just a counter value.

Feedback on these kind of design aspects and/or other ideas are welcome.

nicolasburtey commented 2 years ago

@alexbosworth @ryanthegentry this features would really be helpful for us

nicolasburtey commented 2 years ago

ErikEk commented 2 years ago

@carlaKC Are you still working on this? I could write a PR if you want.

alexbosworth commented 2 years ago

A workaround is to subscribe to all invoices and then individually subscribe to the invoices where you want hold state updates

nicolasburtey commented 2 years ago

A workaround is to subscribe to all invoices and then individually subscribe to the invoices where you want hold state updates

this doesn't work for our use case:

we have one pod creating the invoices, and another pod that is listening to their settlement. we really want to have the second pod listening to invoice settlement continuously without having to pass message around between pods.

alexbosworth commented 2 years ago

There should be no need for message passing since an event is triggered for all subscriptions when invoices are created

nicolasburtey commented 2 years ago

so,

pod1 "invoice-listeners" start, run subcribeToInvoices pod2 "invoice-creator" start, run periodically invoiceCreate

pod1 will receive event even for invoice created after subcribeToInvoices has started?

alexbosworth commented 2 years ago

Yes exactly that should just work because subscribing to invoices means getting an event on any new invoice

nicolasburtey commented 2 years ago

ok, I thought this was only for already existing invoices. this could work then.

do you know if there could be issues listening to 10k+ invoices?

alexbosworth commented 2 years ago

I don't think there would be any issue from the LND perspective, although you can of course cancel the single invoice subscription after it's settled or expired/canceled so I would use that to minimize overall subscriptions

carlaKC commented 2 years ago

@carlaKC Are you still working on this? I could write a PR if you want.

@ErikEk I'm not currently working on this, would be very happy to review a PR though!

ErikEk commented 1 year ago

Been playing around with different solutions here. If we would consider adding partial payment notifications, et al @joostjager, I guess we have to send the full invoice everytime, but with the latest htlc list. Seems bloated but I can't come up with a better solution. I like the idea of adding a timestamp parameter to specify which invoices to return from the subscription.

dolcalmi commented 10 months ago

@ErikEk @Roasbeef any update on this?

nicolasburtey commented 7 months ago

bumping this up. this is why it's a significant issue on our end:

we are using hodl invoice because we want to some logic before settling an invoice
it takes multiple minutes to loop through all invoices to register to back to them after a server restart (our server talking to lnd, not necessarily restarting lnd)
we heavily rely on CD, and put to prod new code 5+ times per day
every time our servers restarts, we have to loop through all those invoices again
during this "restart time", invoice will not be settled as the htlc arrives, which translate to a bad user experience for our users
in a way, the more we push code, the more negative the user experience for our users / user of the Galoy stack is

having a single subscription for all invoices would solve this issue

Roasbeef commented 7 months ago

Hi, we're aware of the need here, thanks for that additional anecdote. We have a large project to overhaul the invoice system at the data storage level which sort of blocks this, as we'd want to ensure we can give back filled notifications as normal. Any review+testing of those PRs (#6288) is appreciated.

After fixing some bugs in this area earlier this year, I spun out this issue that outlines a number of issues with the internals of the current notification system and potential ways to address them: https://github.com/lightningnetwork/lnd/issues/8023

AndySchroder commented 3 weeks ago

Any update on the progress of this one?

Roasbeef commented 2 weeks ago

@AndySchroder no change yet. With the native SQL backend for invoices complete however, we're now in position to easily implement this feature, but only for the native SQL backend.

AndySchroder commented 2 weeks ago

Is the SQL back end going to require more setup from the user, or does it "just work"?

Roasbeef commented 1 week ago

Is the SQL back end going to require more setup from the user, or does it "just work"?

For sqlite, it'll "just work", as it's an embedded database. For postgres, it'll require a bit more work, as the user also needs to run a database management server. There are hosted solutions that make things easier, like Amazon RDS as an example: https://aws.amazon.com/rds/postgresql/ .

lightningnetwork / lnd