Network arg rework by rustyrussell · Pull Request #1450 · ElementsProject/lightning

rustyrussell · 2018-05-03T05:21:56Z

Our network args were a mess, as Tor and local socket options have made clear.

What do people think of going in this direction?

ZmnSCPxj

Generally like the new organization. I think most humans will be unable to remember +=bind only, -=announce only, though. Perhaps better to have something like:

--bind-addr=a        listen on address a
--announce-addr=a    announce our address as a to peers
--addr=a             equivalent to --bind-addr=a --announce-addr=a

ZmnSCPxj · 2018-05-03T06:01:24Z

 	else
 		log_debug(ld->log, "Not guessing addresses: %s",
-			  ld->portnum ? "manually set" : "port set to zero");
+			  (ld->portnum && ld->listen) ? "manually set" : "port set to zero");


Since ld->listen is now involved, it may seem strange if user sets portnum to nonzero and then set --offline and then get the log message "port set to zero". Maybe better "listening disabled"?

ZmnSCPxj · 2018-05-03T06:08:18Z

+enum addr_listen_announce {
+	ADDR_LISTEN = 1,
+	ADDR_ANNOUNCE = 2,
+	ADDR_LISTEN_AND_ANNOUNCE = 3


We take advantage that ADDR_LISTEN_AND_ANNOUNCE == ADDR_LISTEN | ADDR_ANNOUNCE. Maybe it would be better to be explicit about that here?

enum addr_listen_announce { ADDR_LISTEN = 1 << 0, // be explicit that we are using bits ADDR_ANNOUNCE = 1 << 1, ADDR_LISTEN_AND_ANNOUNCE = ADDR_LISTEN | ADDR_ANNOUNCE // be explicit that this is combination of bits. };

Otherwise it may naively seem that numbers are just incrementing.

ZmnSCPxj · 2018-05-03T06:19:58Z

 gossipctl_init,,lfeatures,lflen*u8
 gossipctl_init,,num_wireaddrs,u16
 gossipctl_init,,wireaddrs,num_wireaddrs*struct wireaddr
+gossipctl_init,,announce_listen,num_wireaddrs*u32


While u32 on most systems using gcc will match enum, C99 standard 6.7.2.2 is:

Each enumerated type shall be compatible with char, a signed integer type, or an unsigned integer type. The choice of type is implementation-defined, but shall be capable of representing the values of all the members of the enumeration.

This may bite us later. Under gcc sizeof(enum foo) == sizeof(int) and in most systems sizeof(int) == sizeof(uint32_t).

Yeah, that was kind of a hack (though you'd get a compiler warning if it was wrong). We can actually provide proper mashalling for this type.

Also nit: you consistently use listen_announce elsewhere, and only this csv file uses announce_listen.

ZmnSCPxj · 2018-05-03T06:28:40Z

@@ -656,7 +656,7 @@ def test_connect_by_gossip(self):
        l3port = self.node_factory.get_next_port()
        l3 = self.node_factory.get_node(options={"addr": "127.0.0.1:{}".format(l3port)})


This special casing can be removed now I think? The code below was added in tests/utils.py:

+ 'addr': '127.0.0.1:{}'.format(port),

Yes, good point.

I just stumbled over this myself, while parallelizing node startups, would be nice to remove this.

Saibato · 2018-05-03T09:06:50Z

--addr can then easily also be used when the tor PR reemerges for tor addresses 👍

but dev_hsm_seed I would keep in, its easy to guess when c-ln started first to create initial hsm which in fact is not only a seed it is in fact the key.
I prefer to provide the option to set our own bip32 start and not to mandate to trust PRNG

rustyrussell · 2018-05-03T12:29:44Z

Documentation is definitely required. I was originally going for separate options, before I used prefixes. I think I like it better: it's then clear that you can only use --bind-add with local sockets, and only --announce-addr with Tor.

I will revise tomorrow based on the feedback here, thanks!

cdecker

Nice cleanups, just two minor comments.

cdecker · 2018-05-03T19:51:36Z

+						daemon);
+			break;
+		case ADDR_TYPE_PADDING:
+			/* Shouldn't happen. */


Could be fatal?

cdecker · 2018-05-03T19:54:26Z

        l1 = self.node_factory.get_node(may_reconnect=may_reconnect)
        l2 = self.node_factory.get_node(may_reconnect=may_reconnect)
-        ret = l1.rpc.connect(l2.info['id'], 'localhost', l2.info['port'])
+        ret = l1.rpc.connect(l2.info['id'], 'localhost', l2.info['address'][0]['port'])


We should just shortcut this and make the port an attribute of the LightningNode, no need to keep the structure of the getinfo call

We should actually use an ephemeral port for testing.

I thing we should enhance --addr to allow:

Hostname variants:

`` (empty string) - All IPv4 & IPv6 interfaces. This would be default unless of course --autolisten=0. Announce if addresses seem routable.

0.0.0.0: All IPv4 interfaces. Announce if address seems routable.

::: All IPv6 interfaces. Announce if address seems routable.

Port variants:

``: (empty string) default port, ie. 9735.

:0: an ephemeral port.

This is more of a rework though, since guess_addresses needs to move into gossipd, when then needs to feed the actual address results back to the master for getinfo.

ZmnSCPxj · 2018-05-04T03:39:45Z

 				 "Alias for --network=testnet");
 	opt_register_early_noarg("--mainnet", opt_set_mainnet, ld,
-				 "Alias for --network=bigtcoin");
+				 "Alias for --network=bitcoin");


But my premine!!

ZmnSCPxj · 2018-05-04T03:49:00Z

        # Force l3 to give its address.
        l3port = self.node_factory.get_next_port()
-        l3 = self.node_factory.get_node(options={"ipaddr": "127.0.0.1:{}".format(l3port)})
+        l3 = self.node_factory.get_node(options={"addr": "127.0.0.1:{}".format(l3port)})


Previous comment not yet covered? This can be replaced outright now since we now use addr in the factory, which also adds the address in the announcement now; basically the addr specification here is now redundant.

- l3port = self.node_factory.get_next_port() - l3 = self.node_factory.get_node(options={"addr": "127.0.0.1:{}".format(l3port)}) + l3 = self.node_factory.get_node()

We can also remove the get_next_port method in the factory too, since I added that only to be able to get at the port number for l3, and paving the way for ephemeral ports.

Yes, decided to leave that for the ephemeral port changes.

I have a patch that randomizes ports and allows parallel tests, I'll clean up and open a or tomorrow

@cdecker, I like the current system where if I have this:

l1 = self.node_factory.get_node() l2 = self.node_factory.get_node() l3 = self.node_factory.get_node()

In the logs, I can filter:

lightningd(16331) -> l1 lightningd(16332) -> l2 lightningd(16333) -> l3

So even with randomized ports, I think it would be nice if each test could show the index of the lightningd easily in the logs.

One possibility would be to have a node-factory-factory in the self object. This allocates IDs in batches of 100. Then a node-factory created for each individual test allocates an ID for each created lightningd. So for example one test would have l1 -> lightningd(101), l2 -> lightningd(102)... and another test would have l1 -> lightningd(201), l2 -> lightningd(202)...

Yep, I have added a lightningd-{node_id} prefix that'll allow to filter by a specific node. See commit 3b33a5c for details

It's broken anyway. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@cdecker

We're about to change the JSONRPC, so let's put an explicit 'port' into our node class. We initialize it at startup time: in future I hope to use ephemeral ports to make our tests more easily parallelizable. Suggested-by: @cdecker Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

We set no_reconnect with --offline, but that doesn't work if !DEVELOPER. Make the flag positive, and non-DEVELOPER mode for gossipd. We also don't override portnum with --offline, but have an explicit 'listen' flag. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

We're going to add sockets, and later onion addresses, so the current name is bad. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

It's become clear that our network options are insufficient, with the coming addition of Tor and unix domain support. Currently: 1. We always bind to local IPv4 and IPv6 sockets, unless --port=0, --offline, or any address is specified explicitly. If they're routable, we announce. 2. --addr is used to announce, but not to control binding. After this change: 1. --port is deprecated. 2. --addr controls what we bind to and announce. 3. --bind-addr/--announce-addr can be used to control one and not the other. 4. Unless --autolisten=0, we add local IPv4 & IPv6 port 9735 (and announce if they are routable). 5. --offline still overrides listening (though announcing is still the same). This means we can bind to as many ports/interfaces as we want, and for special effects we can announce different things (eg. we're sitting behind a port forward or a proxy). What remains to implement is semi-automatic binding: we should be able to say '--addr=0.0.0.0:9999' and have the address resolve at bind time, or even '--addr=0.0.0.0:0' and have the port autoresolve too (you could determine what it was from 'lightning-cli getinfo'. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Now we only bind to addresses in our wireaddrs array, we would not autobind to local sockets if they couldn't reach google's nameserver. That's clearly wrong: we should only not bind if there's a protocol issue (eg. no IPv6 support). Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

It does all the other address handling, do this too. It also proves useful as we clean up wildcard address handling. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@icota

This was something @icota implemented, but it fits logically into this cleanup series. We create a new type which is the internal generalization of a wireaddr (which is defined by the spec), and add a case here for a socket name. Based-on-the-true-story-by: @icota Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

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>

1. Add special option where an empty host means 'wildcard for IPv4 and/or IPv6' which means ':1234' can be used to set only the portnum. 2. Only add this protocol wildcard if --autolisten=1 (default) and no other addresses specified. 3. Pass it down to gossipd, so it can handle errors correctly: in most cases, it's fatal not to be able to bind to a port, but for this case, it's OK if we can only bind to one of IPv4/v6 (fatal iff neither). Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

If we're given a wildcard address, we can't announce it like that: we need to try to turn it into a real address (using guess_address). Then we use that address. As a side-effect of this cleanup, we only announce *any* '--addr' if it's routable. This fix means that our tests have to force '--announce-addr' because otherwise localhost isn't routable. This means that gossipd really controls the addresses now, and breaks them into two arrays: what we bind to, and what we announce. That is now what we return to the master for json_getinfo(), which prints them as 'bindings' and 'addresses' respectively. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Someone could try to announce an internal address, and we might probe it. This breaks tests, so we add '--dev-allow-localhost' for our tests, so we don't eliminate that one. Of course, now we need to skip some more tests in non-developer mode. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

…_by_gossip. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell · 2018-05-07T04:31:33Z

OK, rebased, and more clean ups. I included @icota 's unix socket support, but not yet @Saibato 's Tor support (that will be a separate PR).

comments covered

cdecker · 2018-05-07T11:23:54Z

ACK 8089103

ZmnSCPxj · 2018-05-07T14:09:17Z

ACK 8089103

rustyrussell requested review from ZmnSCPxj, cdecker and practicalswift May 3, 2018 05:21

ZmnSCPxj reviewed May 3, 2018

View reviewed changes

cdecker reviewed May 3, 2018

View reviewed changes

rustyrussell force-pushed the network-arg-rework branch 4 times, most recently from 5a7a802 to e46f5b0 Compare May 4, 2018 02:43

ZmnSCPxj previously requested changes May 4, 2018

View reviewed changes

ZmnSCPxj mentioned this pull request May 5, 2018

pytest: Enable parallel test execution and start migration to modern fixtures #1460

Merged

rustyrussell added 14 commits May 7, 2018 12:11

tests/utils.py: remove unused connect routine.

2860ad5

It's broken anyway. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

lightningd: deprecate --ipaddr in favor of --addr.

c303b82

We're going to add sockets, and later onion addresses, so the current name is bad. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

wireaddr: helpers to convert to/from IPv4/v6 addresses.

6953242

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

gossipd: take over address determination, from master.

5b33adf

It does all the other address handling, do this too. It also proves useful as we clean up wildcard address handling. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

test_lightningd.py: remove gratuitous 'addr' option from test_connect…

8089103

…_by_gossip. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell force-pushed the network-arg-rework branch from e46f5b0 to 8089103 Compare May 7, 2018 04:30

rustyrussell mentioned this pull request May 7, 2018

Add support for local socket connections #1323

Closed

rustyrussell changed the title ~~RFC, WIP: Network arg rework~~ Network arg rework May 7, 2018

rustyrussell mentioned this pull request May 7, 2018

Tor support #1471

Merged

ZmnSCPxj approved these changes May 7, 2018

View reviewed changes

cdecker merged commit 79902b4 into ElementsProject:master May 7, 2018

cdecker mentioned this pull request May 8, 2018

A number of pytest fixes and a minor cleanup in gossip #1474

Merged

pooleja mentioned this pull request May 12, 2018

STATUS_FAIL_INTERNAL_ERROR: Failed to bind on 2 socket: Cannot assign requested address #1496

Closed

icota mentioned this pull request Jun 11, 2020

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

Closed

		@@ -656,7 +656,7 @@ def test_connect_by_gossip(self):
		l3port = self.node_factory.get_next_port()
		l3 = self.node_factory.get_node(options={"addr": "127.0.0.1:{}".format(l3port)})

Conversation

rustyrussell commented May 3, 2018

Uh oh!

ZmnSCPxj left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Saibato commented May 3, 2018

Uh oh!

rustyrussell commented May 3, 2018

Uh oh!

cdecker 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!

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 7, 2018

Uh oh!

cdecker commented May 7, 2018

Uh oh!

ZmnSCPxj commented May 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ZmnSCPxj left a comment •

edited

Loading