invoicesrpc: specify private hops for addinvoice, addholdinvoice #3022

Crypt-iQ · 2019-04-28T12:19:43Z

This PR allows a user to manually specify private channels to be used as hop hints in both the addinvoice and addholdinvoice RPC calls via the new --private_hops flag which accepts a JSON array of private channel ids. The new flag must be used in conjunction with the --private flag. In the current implementation, users will advertise all private channels which isn't ideal for privacy-conscious users.

I also changed the --private flag from BoolTFlag to BoolFlag so that users aren't always leaking their private channels if they don't want to.

Usage:

addinvoice
lncli addinvoice --private_hops '[467292441870336, 447501232570368, 487083651170304]' --private
addholdinvoice
lncli addholdinvoice <hash> --private_hops '[467292441870336, 447501232570368, 487083651170304]' --private

joostjager

In terms of commit structure, it may be better to just make two commits: one for lnd and one for lncli building off of that.

joostjager · 2019-07-02T15:02:21Z

cmd/lncli/commands.go

+		cli.StringFlag{
+			Name: "private_hops",
+			Usage: "a json array string of private channel ids " +
+				"to use as hop hints",


Why not the simpler format --private_hops 100,200,300?

joostjager · 2019-07-02T15:06:25Z

cmd/lncli/invoicesrpc_active.go

+			Name: "private_hops",
+			Usage: "a json array string of private channel ids " +
+				"to use as hop hints",
+		},


If you want, you can do some reuse here. See commands.go, paymentFlags().

lnrpc/invoicesrpc/addinvoice.go

joostjager · 2019-07-02T15:09:04Z

lnrpc/invoicesrpc/addinvoice.go

+			// any channels not in the array.
+			if len(invoice.PrivateHops) > 0 {
+				match := false
+				for _, privateChan := range invoice.PrivateHops {


This may be better to implement using a map[uint64]struct{}

Agree, done

joostjager · 2019-07-02T15:09:53Z

lnrpc/invoicesrpc/invoices.proto

+    /**
+    The list of private channel ids that will be used as hop hints.
+    */
+    repeated uint64 private_hops = 10 [json_name = "private_hops"];


Wondering whether name should be private_channels? But we also have hop hints...

Changed to private_channels since I think it's more descriptive

joostjager · 2019-07-02T15:56:25Z

Can you also give a bit more background on how exactly you'd be using this? Is the goal to only expose one private channel for every peer connected to, so that the invoice doesn't disclose the combined capacity of all channels to a peer?

Or do you maybe have info on how the sender is going to approach you?

Crypt-iQ · 2019-07-02T17:36:05Z

Currently the commands include all private channels as hop hints by default. Users may not even know that this is happening (I didn't), so I disabled that in this PR. If I only wanted to specify one, or a few, for the sender to use in path finding, this gives me more granular control. Perhaps some of these private channels are unbalanced and could use some balancing. Maybe there is one "very secret" private channel that I don't want exposed via the regular add(hold)invoice command(s) and I don't really care if my other private channels get exposed to the sender.

andronoob · 2019-08-09T11:15:48Z

@joostjager I'm looking for this feature as well. My Internet connection comes with carrier-grade NAT, so simply setting NAT=true didn't seem to work. I have to embed routing hints of the channel counterparty to receive funds. Eclair mobile has done this properly, while LN-App still generates invoices without any routing hints, which cannot be paid.
I think maybe I should open private channels instead of public channels? In my opinion, there should be some protocol layer workaround, which could work like ssh -R 0.0.0.0:9735:127.0.0.1:9735

halseth · 2019-09-18T13:37:01Z

@andronoob Yes, for non-routing nodes you should be using private channels.

andronoob · 2019-09-19T01:57:38Z

@halseth In other words, it's a limitation of LN protocol that NAT can be a problem for routing nodes, therefore routing nodes must figure out how to overcome this problem by themselves, right?

halseth · 2019-09-23T09:35:15Z

A routing node must be connectable, and how that is achieved is not a protocol problem.

therefore routing nodes must figure out how to overcome this problem by themselves, right?

So yes :)

…4 array

…rivate_channels array

carlaKC

Nice addition, thanks @Crypt-iQ!
Just a few nits/ UX questions from my side :)

carlaKC · 2019-12-02T08:08:07Z

lnrpc/invoicesrpc/invoices.proto

@@ -82,6 +82,11 @@ message AddHoldInvoiceRequest {

    /// Whether this invoice should include routing hints for private channels.
    bool private = 9 [json_name = "private"];
+
+    /**
+    The list of private channel ids that will be used as hop hints.


nit: add that these are short channel IDs?
It's apparent from the uint64 when you're used to the rpc, but might be helpful for newer users :)

carlaKC · 2019-12-02T08:09:28Z

cmd/lncli/invoicesrpc_active.go

@@ -6,6 +6,7 @@ import (
 	"context"
 	"encoding/hex"
 	"fmt"
+	"strings"



Might as well blitz this unnecessary newline while you're here?

carlaKC · 2019-12-02T08:14:52Z

lnrpc/invoicesrpc/addinvoice.go

@@ -253,6 +258,16 @@ func AddInvoice(ctx context.Context, cfg *AddInvoiceConfig,
 		options = append(options, zpay32.CLTVExpiry(uint64(defaultDelta)))
 	}

+	if len(invoice.PrivateChannels) > 0 && !invoice.Private {


Wouldn't the presence of a list of private channels imply that the user wants to have a private invoice?
Non-critical, but I can definitely see myself calling with a list of channels and forgetting the private flag and wanting it to be set anyway.

carlaKC · 2019-12-02T08:21:58Z

lnrpc/invoicesrpc/addinvoice.go

+				// the list of hop hints.
+				sid := channel.ShortChannelID.ToUint64()
+				if _, ok := privateChanMap[sid]; !ok {
+					continue


As is, users can provide public or already closed channels in this list and they will just be filtered out by this + the isPublic check, which could result in there being no hints added when the user gives bad input?

The extreme case would be to error if they provide hops and we end up using none, but maybe just add some logging along the lines of private invoice: received {len} suggested channels, created: {numHints} hints?

Gotcha, agree

carlaKC · 2019-12-02T08:22:52Z

lnrpc/rpc.proto

@@ -2242,6 +2242,11 @@ message Invoice {

    /// List of HTLCs paying to this invoice [EXPERIMENTAL].
    repeated InvoiceHTLC htlcs = 22 [json_name = "htlcs"];
+
+    /**
+    The list of private channel ids that will be used as hop hints.


Add a comment that hop hints will be strictly limited to these channels if they are specified?

alexbosworth · 2019-12-13T20:59:05Z

A potential workaround for this issue now exists with the signMessage RPC API, you can encode your own BOLT 11 payment request however you like and then use that API to craft the signature for the payment request

Crypt-iQ · 2019-12-14T14:57:25Z

A potential workaround for this issue now exists with the signMessage RPC API, you can encode your own BOLT 11 payment request however you like and then use that API to craft the signature for the payment request

I agree with this, but I think that might be complicated for new users? Also at a minimum I think the private hops should not be specified by default if possible

halseth · 2020-09-11T11:24:15Z

@Crypt-iQ Can we close in favor of #3672?

Roasbeef added P3 might get fixed, nice to have routing rpc Related to the RPC interface labels Jun 18, 2019

Roasbeef requested a review from joostjager July 2, 2019 02:05

joostjager reviewed Jul 2, 2019

View reviewed changes

wpaulino mentioned this pull request Nov 4, 2019

Private channel can't receive despite being notified as active. #3660

Closed

halseth mentioned this pull request Nov 7, 2019

Allow user specify routing hints in private invoice. #3672

Merged

halseth requested a review from carlaKC November 7, 2019 11:29

ottosuess mentioned this pull request Nov 7, 2019

Unscanable QR code LN-Zap/zap-iOS#303

Open

Crypt-iQ added 2 commits November 7, 2019 10:58

lnrpc+rpcserver: add support for passing in private_channels as uint6…

f420885

…4 array

cmd: add lncli addinvoice and addholdinvoice support for passing in p…

c40e926

…rivate_channels array

Crypt-iQ force-pushed the private_invoice_0427 branch from 861cc07 to c40e926 Compare November 7, 2019 15:59

carlaKC reviewed Dec 2, 2019

View reviewed changes

Roasbeef added this to the 0.10.0 milestone Jan 14, 2020

Roasbeef added the v0.10 label Jan 17, 2020

Roasbeef removed this from the 0.10.0 milestone Feb 18, 2020

Roasbeef removed the v0.10 label Feb 26, 2020

Crypt-iQ closed this Sep 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

invoicesrpc: specify private hops for addinvoice, addholdinvoice #3022

invoicesrpc: specify private hops for addinvoice, addholdinvoice #3022

Crypt-iQ commented Apr 28, 2019 •

edited

Loading

joostjager left a comment •

edited

Loading

joostjager Jul 2, 2019

Crypt-iQ Nov 7, 2019

joostjager Jul 2, 2019

joostjager Jul 2, 2019

Crypt-iQ Nov 7, 2019

joostjager Jul 2, 2019

Crypt-iQ Nov 7, 2019

joostjager commented Jul 2, 2019

Crypt-iQ commented Jul 2, 2019

andronoob commented Aug 9, 2019 •

edited

Loading

halseth commented Sep 18, 2019

andronoob commented Sep 19, 2019

halseth commented Sep 23, 2019

carlaKC left a comment

carlaKC Dec 2, 2019

carlaKC Dec 2, 2019

carlaKC Dec 2, 2019

carlaKC Dec 2, 2019

Crypt-iQ Dec 5, 2019

carlaKC Dec 2, 2019

alexbosworth commented Dec 13, 2019

Crypt-iQ commented Dec 14, 2019

halseth commented Sep 11, 2020

invoicesrpc: specify private hops for addinvoice, addholdinvoice #3022

invoicesrpc: specify private hops for addinvoice, addholdinvoice #3022

Conversation

Crypt-iQ commented Apr 28, 2019 • edited Loading

joostjager left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager commented Jul 2, 2019

Crypt-iQ commented Jul 2, 2019

andronoob commented Aug 9, 2019 • edited Loading

halseth commented Sep 18, 2019

andronoob commented Sep 19, 2019

halseth commented Sep 23, 2019

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexbosworth commented Dec 13, 2019

Crypt-iQ commented Dec 14, 2019

halseth commented Sep 11, 2020

Crypt-iQ commented Apr 28, 2019 •

edited

Loading

joostjager left a comment •

edited

Loading

andronoob commented Aug 9, 2019 •

edited

Loading