Allow user specify routing hints in private invoice. #3672

roeierez · 2019-11-04T07:43:32Z

This PR changes the AddInvoice/AddHoldInvoice RPC endpoints to be able to get routing hints and populate them in the resulted invoice.
In fact the RoutingHints already existed in both lnrpc.Invoice and lnrpc.AddHoldInvoiceRequest but were ignored and this change only adds them to the invoice.

This is useful in the following cases:

When ones want to expose specific private channels, or to get payment via them.
When the default routing hints logic skips some channels due to incomplete graph state (see issue Private channel can't receive despite being notified as active. #3660). This is specifically important to enable receiving funds on mobile wallets that just bootstrapped with a new channel.

halseth · 2019-11-07T10:20:50Z

Can be consolidated with #3022? Or is it different?

roeierez · 2019-11-07T10:44:59Z

Can be consolidated with #3022? Or is it different?

Yes, needs to. I didn't see the other PR when opened this. I Actually think the approach there is better (user needs to only pass channel ids as hops).
I believe what made me walk this path is that I saw routing_hints field in AddHoldInvoiceRequest, worth removing it as part of #3022.

roeierez · 2019-11-07T18:20:36Z

@halseth Looking again at #3022 it won't be usable for the use case in this PR. In #3022 the private channels passed only serve as filter while at this PR they are given by the user explicitly.

halseth · 2019-11-11T09:33:30Z

in what cases will it be useful to add hints for channels not known by the receiver (and hence the #3022 approach won't work)?

EDIT: Just read the latest response on #3660, and see that it could be useful in the case where the remote node info is not yet known.

roeierez · 2019-11-11T10:19:21Z

EDIT: Just read the latest response on #3660, and see that it could be useful in the case where the remote node info is not yet known.

Yes that's the case.

carlaKC

Catching up on the overlap between this and #3022:
3022: User provides a list of channels which restricts which channels we use to create hop hints, with the intention of limiting which channels are used for hop hints for balancing reasons
3672: User specifically provides the hop hints they need, to cover the case where the current logic does not allow creating a hint for the desired channel yet

@roeierez what are your thoughts on limiting the hints used for invoices to the route hints provided by the user? This would cover the use case of #3022 because the user can just provide hints for the channels they want to include.

There is a UX consideration of having to construct hints yourself wrt work for the user + potential for error. Generally, it would be nice to be able to check that the user provided hints are ok, but not in the case of #3660. We could potentially address this with a getHopHint endpoint which will produce a hop hint for a given channel and perform validation on the channel? Most users can query this endpoint to get hints for the channels which they want to use, and in special use cases you can just roll your own?

Tagging @Crypt-iQ, author of #3022, to loop him into this discussion.

roeierez · 2019-12-02T09:36:23Z

@roeierez what are your thoughts on limiting the hints used for invoices to the route hints provided by the user? This would cover the use case of #3022 because the user can just provide hints for the channels they want to include.

I am fine with this approach.

There is a UX consideration of having to construct hints yourself wrt work for the user + potential for error. Generally, it would be nice to be able to check that the user provided hints are ok, but not in the case of #3660. We could potentially address this with a getHopHint endpoint which will produce a hop hint for a given channel and perform validation on the channel? Most users can query this endpoint to get hints for the channels which they want to use, and in special use cases you can just roll your own?

I actually like the approach of #3022 where the user is only pass channel ids as input instead of RouteHint structures and your arguments explains that well.
I would suggest to go with the #3022 with some small changes to solve this use case as well.

I can think of two approaches for such adaptation:

If user passes explicit channel ids, then skip validation (public channel) in LND and force populating the route hints.
Add additional flag to force populating the route hints.

Since this is a bit "advanced" use case I tend to go with #1.

Crypt-iQ · 2019-12-05T01:31:09Z

I think either method (3022 or 3672) would work. My motivation for opening 3022 was for privacy reasons since creating an invoice populates all private channels by default with no option for specificity. As long as that is addressed (which both methods would), I'm fine :)

halseth · 2020-07-23T10:50:39Z

@roeierez Do you want to pick this up again? I think we can get this merged addressing the following:

If private == false and the user specifies route hints, add only these route hints when creating the invoice (this should address the privacy concern from invoicesrpc: specify private hops for addinvoice, addholdinvoice #3022).
If private == true, add additional route hints for private channels to the invoice.

This should also let the user specify public channels as route hints, which can be useful in certain cases.

We should also update the RPC documentation to reflect the behaviour.

roeierez · 2020-07-23T14:10:56Z

@roeierez Do you want to pick this up again?

Sounds good. I'll address the feedback.

hsjoberg · 2020-09-10T17:11:50Z

Needs rebase. The route hints code was refactored in #4521.

Unless I'm missing something, but this PR seems more powerful than #3022 as it allows for fake channels and the Pay to Open use-case explained here: #4018. Perhaps the other PR could be reworked to work with this one?

halseth

A few nits otherwise LGTM 👍

lnrpc/invoicesrpc/addinvoice.go

halseth · 2020-09-11T11:19:57Z

lnrpc/invoicesrpc/addinvoice.go

+	usedChannels := make(map[uint64]struct{})
+	for i, h := range invoice.RouteHints {
+		if i >= 20 {
+			break


return error instead?

halseth · 2020-09-11T11:23:10Z

Also needs rebase as pointed out by @hsjoberg

Crypt-iQ · 2020-09-11T12:41:29Z

Closed the other one in favor of this one @roeierez

hsjoberg · 2020-09-11T13:10:34Z

Note that this one currently does not support providing route hints via the CLI, only RPC

Crypt-iQ

I think we'll want lncli to be updated with the same capabilities.

Crypt-iQ · 2020-09-11T15:33:47Z

lnrpc/rpc.proto

@@ -2659,7 +2659,8 @@ message Invoice {

    /*
    Route hints that can each be individually used to assist in reaching the
-    invoice's destination.
+    invoice's destination. The specified routing hints will be included in
+    the invoice.


Comment changes need to re-generate protos as the comments are replicated in other lnrpc files

Removed the comment change, not needed after all.

Crypt-iQ · 2020-09-11T15:33:52Z

lnrpc/invoicesrpc/addinvoice.go

+			break
+		}
+		options = append(options, zpay32.RouteHint(h))
+		usedChannels[h[0].ChannelID] = struct{}{}


I don't think we should index like this - what if h is empty? Why is only the first channel added to usedChannels?

@Crypt-iQ
Added validation for h is empty, missed that so thanks for catching it.
Regarding indexing, I am thinking only the first hop is our direct channel, am I missing something?

Roasbeef · 2020-11-04T02:28:07Z

Is this PR still active? The 0.12 window is closing in a week or so.

halseth · 2020-11-04T09:41:36Z

ping @roeierez

roeierez · 2020-11-05T11:00:15Z

ping @roeierez

Yes, I am here :). Going to pick this up again.

roeierez · 2020-11-06T06:16:26Z

@halseth rebased and adapted to the code changes that have been introduced since the start of this PR.
Also added an integration test.

halseth

Apart from a few nits this LGTM 👯‍♂️

lnrpc/invoicesrpc/addinvoice.go

lntest/itest/lnd_single_hop_invoice_test.go

carlaKC

Great to get this in!

carlaKC · 2020-11-06T13:00:31Z

lnrpc/invoicesrpc/addinvoice.go

+
+	// Optional route hints that can each be individually used to assist in
+	// reaching the invoice's destination.
+	RouteHints [][]zpay32.HopHint


nittynit: godocs shoudl start with the var name RouteHints...

lntest/itest/lnd_single_hop_invoice_test.go

hsjoberg · 2020-11-06T16:12:06Z

utACK @ 4463a4c, although I've tested previous iterations.

roeierez force-pushed the invoice-hints branch from 287d440 to 20c7950 Compare November 4, 2019 09:43

halseth requested a review from carlaKC November 7, 2019 09:25

Roasbeef added enhancement Improvements to existing features / behaviour P3 might get fixed, nice to have payments Related to invoices/payments routing labels Nov 9, 2019

carlaKC reviewed Dec 2, 2019

View reviewed changes

Roasbeef requested a review from halseth July 23, 2020 03:04

Roasbeef added this to the 0.12.0 milestone Jul 23, 2020

roeierez force-pushed the invoice-hints branch from 20c7950 to f55ca59 Compare July 27, 2020 13:18

halseth suggested changes Sep 11, 2020

View reviewed changes

halseth mentioned this pull request Sep 11, 2020

invoicesrpc: specify private hops for addinvoice, addholdinvoice #3022

Closed

halseth requested a review from Crypt-iQ September 11, 2020 12:43

Crypt-iQ reviewed Sep 11, 2020

View reviewed changes

roeierez force-pushed the invoice-hints branch 4 times, most recently from 036ccb5 to a022990 Compare November 6, 2020 05:52

roeierez requested review from halseth, carlaKC and Crypt-iQ November 6, 2020 09:25

halseth approved these changes Nov 6, 2020

View reviewed changes

lnrpc/invoicesrpc/addinvoice.go Outdated Show resolved Hide resolved

lnrpc/invoicesrpc/addinvoice.go Outdated Show resolved Hide resolved

lnrpc/invoicesrpc/addinvoice.go Show resolved Hide resolved

lntest/itest/lnd_single_hop_invoice_test.go Outdated Show resolved Hide resolved

halseth requested review from Crypt-iQ and removed request for Crypt-iQ November 6, 2020 10:12

roeierez force-pushed the invoice-hints branch from a022990 to 16f207b Compare November 6, 2020 10:28

carlaKC approved these changes Nov 6, 2020

View reviewed changes

invoicerpc: Allow user specify routing hints.

4463a4c

roeierez force-pushed the invoice-hints branch from 16f207b to 4463a4c Compare November 6, 2020 15:54

Roasbeef merged commit 99b0913 into lightningnetwork:master Nov 12, 2020

3nprob mentioned this pull request Feb 13, 2021

Specify individual private channels in addinvoice call #2963

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow user specify routing hints in private invoice. #3672

Allow user specify routing hints in private invoice. #3672

roeierez commented Nov 4, 2019

halseth commented Nov 7, 2019

roeierez commented Nov 7, 2019

roeierez commented Nov 7, 2019

halseth commented Nov 11, 2019 •

edited

Loading

roeierez commented Nov 11, 2019

carlaKC left a comment

roeierez commented Dec 2, 2019

Crypt-iQ commented Dec 5, 2019 •

edited

Loading

halseth commented Jul 23, 2020

roeierez commented Jul 23, 2020

hsjoberg commented Sep 10, 2020 •

edited

Loading

halseth left a comment

halseth Sep 11, 2020

halseth commented Sep 11, 2020

Crypt-iQ commented Sep 11, 2020

hsjoberg commented Sep 11, 2020

Crypt-iQ left a comment

Crypt-iQ Sep 11, 2020

roeierez Nov 6, 2020

Crypt-iQ Sep 11, 2020

roeierez Nov 6, 2020

Roasbeef commented Nov 4, 2020

halseth commented Nov 4, 2020

roeierez commented Nov 5, 2020

roeierez commented Nov 6, 2020

halseth left a comment

carlaKC left a comment

carlaKC Nov 6, 2020

hsjoberg commented Nov 6, 2020

Allow user specify routing hints in private invoice. #3672

Allow user specify routing hints in private invoice. #3672

Conversation

roeierez commented Nov 4, 2019

halseth commented Nov 7, 2019

roeierez commented Nov 7, 2019

roeierez commented Nov 7, 2019

halseth commented Nov 11, 2019 • edited Loading

roeierez commented Nov 11, 2019

carlaKC left a comment

Choose a reason for hiding this comment

roeierez commented Dec 2, 2019

Crypt-iQ commented Dec 5, 2019 • edited Loading

halseth commented Jul 23, 2020

roeierez commented Jul 23, 2020

hsjoberg commented Sep 10, 2020 • edited Loading

halseth left a comment

Choose a reason for hiding this comment

halseth Sep 11, 2020

Choose a reason for hiding this comment

halseth commented Sep 11, 2020

Crypt-iQ commented Sep 11, 2020

hsjoberg commented Sep 11, 2020

Crypt-iQ left a comment

Choose a reason for hiding this comment

Crypt-iQ Sep 11, 2020

Choose a reason for hiding this comment

roeierez Nov 6, 2020

Choose a reason for hiding this comment

Crypt-iQ Sep 11, 2020

Choose a reason for hiding this comment

roeierez Nov 6, 2020

Choose a reason for hiding this comment

Roasbeef commented Nov 4, 2020

halseth commented Nov 4, 2020

roeierez commented Nov 5, 2020

roeierez commented Nov 6, 2020

halseth left a comment

Choose a reason for hiding this comment

carlaKC left a comment

Choose a reason for hiding this comment

carlaKC Nov 6, 2020

Choose a reason for hiding this comment

hsjoberg commented Nov 6, 2020

halseth commented Nov 11, 2019 •

edited

Loading

Crypt-iQ commented Dec 5, 2019 •

edited

Loading

hsjoberg commented Sep 10, 2020 •

edited

Loading