Add an option to subscribe to every Hold Invoice at once

[sergioabril]

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

1. Subscribe to subscribeInvoices

2. 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.

3. 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

1. 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.

2. 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]

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]

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 ***@***.***> 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]

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]

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

[nicolasburtey]

related PR: GaloyMoney/blink#1188

[ErikEk]

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

[alexbosworth]

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

[alexbosworth]

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

[nicolasburtey]

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]

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]

@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]

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]

@ErikEk @Roasbeef any update on this?

[nicolasburtey]

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]

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: #8023

[AndySchroder]

Any update on the progress of this one?

[Roasbeef]

@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]

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

[Roasbeef]

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/ .

[ziggie1984]

Hi folks, taking this over as part of the hodlinvoice overhaul,

I wonder if we need to keep this backwards compatibility here, otherwise there will be no way to introduce a new RPC cmd:

lnd/invoices/invoiceregistry.go

Lines 318 to 321 in 8ac184a

	case event := <-i.invoiceEvents:
	// For backwards compatibility, do not notify all
	// invoice subscribers of cancel and accept events.
	state := event.invoice.State

[ziggie1984]

Regarding the partial payment notification, I would first introduce this only for single invoice subscriptions, otherwise the return stream can become pretty bloated as mentioned by @ErikEk