Add support for local socket connections by icota · Pull Request #1323 · ElementsProject/lightning

icota · 2018-04-04T14:37:58Z

This is a flexible way to connect two instances of c-lightning via means other than the official ones. Run one side with --localsocket=lightning-listener and the other simply connects to the full socket path.

In practice two c-lightning instances are run on separate machines with glue code connecting to sockets and forwarding data between them.

I use this to make a lightning connection (on an otherwise disconnected device) via NFC, but can be made to support bluetooth, infrared, sound or even smoke signals, anything really. Use case would be communicating with always-connected payment terminals with a mobile device that might not be.

This is way out of lightning spec but it doesn't seem to hurt anything. Let me know know what you think!

ZmnSCPxj · 2018-04-04T15:40:16Z

 	ADDR_TYPE_PADDING = 0,
 	ADDR_TYPE_IPV4 = 1,
 	ADDR_TYPE_IPV6 = 2,
+	ADDR_TYPE_LOCAL_SOCKET = 5


I am hesitant to put this in struct wireaddr as this is also used in BOLT standard-compliant node_announcement that get broadcast as node gossip.

At minimum the numbering should not be directly after the BOLT-defined ones.

It might be better to bring up this possible use case in lightning-dev mailing list.

Alternatively we could skip this address type when sending gossip announcements.

Agreed, it makes no sense at all to broadcast these. It hadn't occurred to me. I will look into alternatives.

I don't think there's much value in adding local sockets to the BOLTs but I settled on a MIME type (application/lightning) and an application id (LIGHTNING) for NFC payments. I'll bring this up on the ML when I get the time since I'm not sure how to proceed on standardising these.

If we never broadcast these there is no need to bring it up on the mailinglist, but, I am uncertain if we should be using struct wireaddr then, or some other mechanism. Intent, as I understand it, is that struct wireaddr, is for standard broadcastable messages. @rustyrussell @cdecker opinions?

ZmnSCPxj · 2018-04-04T15:53:34Z


 }

+static char *opt_add_localsocket(const char *arg, struct lightningd *ld)


listconfigs is crashing in CI, probably due to not handling this case properly in add_config function.

lightning/lightningd/options.c

Lines 874 to 877 in 1a55147

} else {

/* Insert more decodes here! */

abort();

}

rustyrussell · 2018-04-05T04:28:56Z

It's cute to see these in the local address listings, but the consequences of putting it in struct wireaddr are probably not worth dealing with.

Just add a '--local-socket' option perhaps? And set addr->type to ADDR_TYPE_PADDING.

icota · 2018-04-05T08:33:35Z

@rustyrussell There is the --localsocket option when we start the daemon, to make it listen. Did you mean a JSON option when connecting?

icota · 2018-04-06T09:39:06Z

New code avoids changes to struct_wireaddr opting to just use ADDR_TYPE_PADDING. It's less code and doesn't broadcast.

check-source fails at shellcheck and check fails at test-blockchaintrack. I don't think either of those are my fault. I guess I'm missing something to run these tests correctly.

ZmnSCPxj · 2018-04-06T10:35:54Z

CI is fine so I suppose it is OK. You can install shellcheck, most distros seem to have it. test_blockchaintrack is known flaky and rerunning it should be OK.

Now reviewing code, please wait...

ZmnSCPxj · 2018-04-06T10:39:33Z

 			      portnum);
+
+	/* Optional UNIX socket */
+	if (localsocket) {


This comes from the fromwire_gossipctl_init. In towire_gossipctl_init a NULL pointer is converted to a 0-length array on-the-wire, I am uncertain if a 0-length array on-the-wire will be converted by fromwire_gossipctl_init to a NULL pointer. I shall check later when I can get access to my hacking node.

No issue here: turns out 0-length arrays on-the-wire are converted to NULL by fromwire_* functions, so ok.

ZmnSCPxj · 2018-04-06T10:43:47Z

 	io_new_conn(reach, fd, conn_init, reach);
 }

+static void try_connect_local_socket(struct daemon *daemon, const struct pubkey *id, const struct addrhint *a)


Might conflict with ongoing changes in #1322, that is higher priority than this I think, so please monitor that PR when it gets merged in and see if this gets conflicted (maybe not a text-level conflict but some of the functions this try_connect_local_socket calls might change their signatures and exact behavior). The first thing I can think of is that reach->succeeded has been removed in #1322, but please see that PR more for other changes that might affect this code.

ZmnSCPxj · 2018-04-06T13:30:15Z

ACK 61cece6

Note however that #1322 changes code that this PR depends on, so we should take care. I somewhat prefer that we get #1322 merged first, then adapt this PR to it, but anyway adapting is needed at some point.

icota · 2018-04-06T13:39:59Z

Thanks @ZmnSCPxj! No big deal if you merge #1322 first, this one is smaller so easier to adapt

cdecker · 2018-04-15T15:44:08Z

Holding this PR back a bit further since #1322 is rather high priority and I don't want to delay by introducing conflicts. Just wanted to acknowledge that we haven't forgotten about this excellent change @icota 😉

ZmnSCPxj · 2018-04-26T08:48:23Z

@icota could you please rebase? #1322 has been merged.

Needs rebase, #1322 probably strongly affects this PR

icota · 2018-04-29T16:05:49Z

Rebased, waiting to see if CI is happy

ZmnSCPxj

Mostly request to change in gossipd/gossip_wire.csv. Also suggestion for localsocket arg of setup_listeners.

ZmnSCPxj · 2018-04-30T03:29:17Z

 }

-static void setup_listeners(struct daemon *daemon, u16 portnum)
+static void setup_listeners(struct daemon *daemon, u16 portnum, u8 *localsocket)


localsocket is only ever read in this function, and is effectively an input string. Maybe better to write as const char * and move the type conversion from u8* to char * to caller of setup_listeners. Or at least const u8* in the argslist.

ZmnSCPxj · 2018-04-30T03:30:34Z

 # Gossipd->master, I am ready.
 gossipctl_init_reply,3100

+gossipctl_init,,lslen,u16


Should this not be with the gossipctl_init fields before gossipctl_init_reply? Clearer that way.

ZmnSCPxj · 2018-04-30T03:38:24Z

Sorry @icota but a new commit in #1440 also conflicts with this: specifically the second commit by @rustyrussell moves setup of listeners to after the activate message, so the localsocket argument in init needs to be moved to the new activate.

icota · 2018-04-30T12:13:50Z

No worries @ZmnSCPxj, local sockets are kind of a niche application so obviously important stuff needs to go in first. I've rebased yet again.

ZmnSCPxj · 2018-04-30T12:40:39Z

ACK 4ae3aaa

icota · 2018-05-01T07:10:04Z

make check fails at test_pay_disconnect which doesn't seem to be related at all:

Traceback (most recent call last):
  File "tests/test_lightningd.py", line 4280, in test_pay_disconnect
    l1.daemon.wait_for_log(r'Peer permanent failure in CHANNELD_NORMAL: lightning_channeld: received ERROR channel .*: update_fee 150000 outside range 4250-75000')

ZmnSCPxj · 2018-05-01T07:28:33Z

Kicked CI, hopefully it is a temporary thing.

cdecker · 2018-05-01T16:52:24Z

+		addr->type = ADDR_TYPE_PADDING;
+		addr->addrlen = strlen(ip);
+		addr->port = 9999;
+		memcpy(&addr->addr, ip, addr->addrlen);


Is this missing an addrlen < sizeof(addr->addr) check?

cdecker · 2018-05-01T16:54:18Z

+		addr.type = ADDR_TYPE_PADDING;
+		addr.addrlen = sizeof(sun->sun_path);
+		memcpy(addr.addr, &sun->sun_path, addr.addrlen);
+		addr.port = 9999;


I think 0 as magic value might be better here, just in case we use the port somewhere.

cdecker · 2018-05-01T16:54:59Z

Just two last nits, happy to merge this soon ^^

icota · 2018-05-01T19:15:14Z

I've amended the code, I'm thinking this might be the day 🤞

icota · 2018-05-01T19:47:24Z

Foiled by CI test_closing_different_fees 😢

rustyrussell

That got ugly fast! You can't mess with how we unmarshal ADDR_TYPE_PADDING; it's in the spec as zero length.

This is internal-only between the main daemon and gossipd. Ugly as it is, I prefer that to be explicit, rather than using wireaddr for this. Otherwise we'll risk getting local file accesses because someone gossiped a socket addresses to us!

Add a new message to gossipd 'towire_gossipctl_peer_addrhint_sockname' which takes a socket name; overloading the wireaddr to add local sockets gives me the creeps, since we also accept them from peers.
Then 'struct addrhint' needs to change in gossipd, to add a 'char *sockname' and appropriate handling.
Finally opt_add_addr needs enhancement, to put the socketname string somewhere if it starts with / (or, use a different option name).

rustyrussell · 2018-05-02T03:08:49Z

 # If non-zero, port to listen on.
 gossipctl_activate,,port,u16
+gossipctl_activate,,lslen,u16
+gossipctl_activate,,localsocket,lslen*u8


You can use 'wirestring' here, which is much nicer, and removes cast (it's actually a char*, but wirestring is the name for purposes of the csv).

rustyrussell · 2018-05-02T03:09:14Z

 	char *rpc_filename;

+	/* Local socket for incoming connections */
+	u8 *localsocket_filename;


rustyrussell · 2018-05-02T03:13:00Z

 		addr->addrlen = 16;
 		break;
+	case ADDR_TYPE_PADDING:
+		addr->addrlen = ((u8)*max) - 2;


This is definitely wrong! We can have things in the msg after the addr. The ADDR_TYPE_PADDING case needs to simply 'return true' (addrlen is 0, there's no port).

... and set addr->addrlen = 0.

rustyrussell · 2018-05-02T03:49:56Z

Actually, after more thought, you can just introduce a new 'wireaddr_or_socket' type, and wire that up in the various places...

ZmnSCPxj · 2018-05-02T04:36:42Z

Agree with the wireaddr_or_socket type idea, but maybe name to contactaddr?

struct contactaddr {
    enum contactaddr_type type;
    union {
        struct wireaddr wireaddr;
        char *localsocket;
    } u;
};

This would probably require special marshaling towire_ and fromwire_ code, though.

rustyrussell · 2018-05-02T05:18:56Z

I have half a patch to do this properly. Give me 2 hours please, will push.

rustyrussell · 2018-05-02T11:41:12Z

Yuck, this ended up quite invasive. There are some nasty places to fix, too.

I've got a very rough cut here:
https://github.com/rustyrussell/lightning/tree/guilt/pr-1323

It's a bit weird since it's built on this PR then ends up reworking much of it: it might be cleaner to combine into one logical sequence.

ZmnSCPxj · 2018-05-02T13:13:05Z

@rustyrussell your branch looks fine to me, although I mildly agree to combine to a better logical sequence. I would have moved the "single localsocket -> multiple wireaddr_or_sockname and make local sockets in a loop" conversion to its own commit (i.e. leave the localsocket in gossipctl_activate in the big wireaddr to wireaddr_or_sockname move).

This replacement is a little menial, but it explicitly catches all the places where we allow a local socket. The actual implementation of opening a AF_UNIX socket is almost hidden in the patch. The detection of "valid address" is now more complex: p->addr.itype != ADDR_INTERNAL_WIREADDR || p->addr.u.wireaddr.type != ADDR_TYPE_PADDING But most places we do this, we should audit: I'm pretty sure we can't get an invalid address any more from gossipd (they may be in db, but we should fix that too). Closes: ElementsProject#1323 Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell · 2018-05-07T07:29:10Z

OK, This has been swallowed into the "crap, where did my time go!" PR #1450...

This replacement is a little menial, but it explicitly catches all the places where we allow a local socket. The actual implementation of opening a AF_UNIX socket is almost hidden in the patch. The detection of "valid address" is now more complex: p->addr.itype != ADDR_INTERNAL_WIREADDR || p->addr.u.wireaddr.type != ADDR_TYPE_PADDING But most places we do this, we should audit: I'm pretty sure we can't get an invalid address any more from gossipd (they may be in db, but we should fix that too). Closes: ElementsProject#1323 Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

ZmnSCPxj suggested changes Apr 4, 2018

View reviewed changes

icota force-pushed the offline-payments branch from c1fe67d to 61cece6 Compare April 6, 2018 09:32

ZmnSCPxj reviewed Apr 6, 2018

View reviewed changes

ZmnSCPxj previously approved these changes Apr 6, 2018

View reviewed changes

icota force-pushed the offline-payments branch from 61cece6 to 373fcef Compare April 29, 2018 15:54

ZmnSCPxj suggested changes Apr 30, 2018

View reviewed changes

Add an optional local socket listener

07fb8f8

icota force-pushed the offline-payments branch from 373fcef to 4ae3aaa Compare April 30, 2018 12:11

ZmnSCPxj approved these changes Apr 30, 2018

View reviewed changes

cdecker reviewed May 1, 2018

View reviewed changes

icota added 2 commits May 1, 2018 21:09

wireaddr: treat address strings starting with '/' as local sockets

11e7e66

gossip: handle incoming local socket connections

44af32a

icota force-pushed the offline-payments branch from 4ae3aaa to 44af32a Compare May 1, 2018 19:10

rustyrussell requested changes May 2, 2018

View reviewed changes

cdecker closed this in 73cd009 May 7, 2018

icota mentioned this pull request Jun 11, 2020

RFC: Introducing AltNet, a pluggable framework for alternative transports bitcoin/bitcoin#18988

Closed


		}

		static char opt_add_localsocket(const char arg, struct lightningd *ld)

Conversation

icota commented Apr 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustyrussell commented Apr 5, 2018

Uh oh!

icota commented Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

icota commented Apr 6, 2018

Uh oh!

ZmnSCPxj commented Apr 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZmnSCPxj commented Apr 6, 2018

Uh oh!

icota commented Apr 6, 2018

Uh oh!

cdecker commented Apr 15, 2018

Uh oh!

ZmnSCPxj commented Apr 26, 2018

Uh oh!

icota commented Apr 29, 2018

Uh oh!

ZmnSCPxj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZmnSCPxj commented Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

icota commented Apr 30, 2018

Uh oh!

ZmnSCPxj commented Apr 30, 2018

Uh oh!

icota commented May 1, 2018

Uh oh!

ZmnSCPxj commented May 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdecker commented May 1, 2018

Uh oh!

icota commented May 1, 2018

Uh oh!

icota commented May 1, 2018

Uh oh!

rustyrussell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustyrussell commented May 2, 2018

Uh oh!

ZmnSCPxj commented May 2, 2018

icota commented Apr 5, 2018 •

edited

Loading

ZmnSCPxj commented Apr 30, 2018 •

edited

Loading