Let gossipd automatically announce the node when a local channel_announcement is queued #413

cdecker · 2017-12-08T12:45:31Z

This closes #320. I'm spinning this one out of the changes that I'm testing for #284 since the bigger PR has a flaky unit test when running under valgrind.

cdecker · 2017-12-12T23:40:26Z

Ok, added @rustyrussell's changes from #430, and skipped the two that were causing a conflict. I'll fix the merge conflict with master tomorrow so we can get this merged :-)

This is used to identify our own announcements. Signed-off-by: Christian Decker <[email protected]>

We are still generating only char* style aliases, but the field is defined to be unicode, which doesn't mix too well with char. Signed-off-by: Christian Decker <[email protected]>

First step towards `gossipd` managing the `node_announcement`. Signed-off-by: Christian Decker <[email protected]>

This will later be used to determine whether or not we should announce ourselves as a node. Signed-off-by: Christian Decker <[email protected]>

Signed-off-by: Christian Decker <[email protected]>

Rather than using the destructor, hook up the cmd so we can close it. peers are allocated off ld, so they are only destroyed explicitly. Signed-off-by: Rusty Russell <[email protected]>

As demonstrated in the test at the end of this series, openingd dying spontaneously causes the conn to be freed which causes the subd to be destroyed, which fails the peer, which hits the db. Signed-off-by: Rusty Russell <[email protected]>

On unmarshal, we stop unmarshaling on a 0 (ADDR_TYPE_PADDING) type. So we should also stop marshaling in that case. Signed-off-by: Rusty Russell <[email protected]>

We can open other channels, if we want. Signed-off-by: Rusty Russell <[email protected]>

Signed-off-by: Rusty Russell <[email protected]>

We should not disconnect from a peer just because it fails opening; we should return it to gossipd, and give a meaningful error. Closes: ElementsProject#401 Signed-off-by: Rusty Russell <[email protected]>

We'll want this for the next change, where gossipd migrates remote peers back to local ones. Signed-off-by: Rusty Russell <[email protected]>

…elds. We should also go through and use consistent nomenclature on functions which are used with a local peer ("lpeer_xxx"?) and those with a remote peer ("rpeer_xxx"?) but this is minimal. Signed-off-by: Rusty Russell <[email protected]>

Signed-off-by: Rusty Russell <[email protected]>

Otherwise we always say it died because we killed it, so we don't get the exit status. Signed-off-by: Rusty Russell <[email protected]>

Signed-off-by: Rusty Russell <[email protected]>

All peers come from gossipd, and maintain an fd to talk to it. Sometimes we hand the peer back, but to avoid a race, we always recreated it. The race was that a daemon closed the gossip_fd, which made gossipd forget the peer, then master handed the peer back to gossipd. We stop the race by never closing the gossipfd, but hand it back to gossipd for closing. Now gossipd has to accept two fds, but the handling of peers is far clearer. Signed-off-by: Rusty Russell <[email protected]>

If the peer is moved from remote to local, this may be lost; it's more secure to increment after we've sent the broadcast.

When gossipd sends a message, have a gossip_index. When it gets back a peer, the current gossip_index is included, so it can know exactly where it's up to. Most of this is mechanical plumbing through openingd, channeld and closingd, even though openingd and closingd don't (currently) read gossip, so their gossip_index will be unchanged. Signed-off-by: Rusty Russell <[email protected]>

It was relying on the message order instead of waiting the desired state. Signed-off-by: Christian Decker <[email protected]>

Signed-off-by: Christian Decker <[email protected]>

cdecker · 2017-12-13T11:37:19Z

Rebased and should be ready for review. I'll also walk through Rusty's changes and review them.

Now using assertRaisesRegex instead of try-except and added restart to nodes. Signed-off-by: Christian Decker <[email protected]>

cdecker

Checked @rustyrussell: LGTM (but can't approve since I opened the PR 😄 )

cdecker · 2017-12-13T11:42:01Z

lightningd/peer_control.c

@@ -183,10 +193,10 @@ void peer_fail_permanent(struct peer *peer, const u8 *msg TAKES)
 	 * zero (ie. all bytes zero), in which case it refers to all
 	 * channels. */
 	static const struct channel_id all_channels;
+	const char *why = tal_strndup(NULL, (char *)msg, tal_len(msg));


This threw me off a bit, but in combination with the TAKES above and a take() on the caller side, this is simply a conditional tal_steal and type cast, or am I missing something?

Yes, strndup will take() if it needs to, which ahem.. takes care of it. :)

cdecker · 2017-12-13T11:56:34Z

lightningd/peer_control.c

+		return 0;
+	}
+
+	/* FIXME: Should we save addr in peer, or should gossipd remember it? */


I'd say just send an address hint to gossipd

cdecker · 2017-12-13T12:04:53Z

tests/test_lightningd.py

+        try:
+            l1.rpc.fundchannel(l2.info['id'], 100000)
+        except ValueError as verr:
+            str(verr).index('to_self_delay {} larger than {}'


These expected exceptions can be implemented using assertRaises or assertRaisesRegex. They're a bit shorter and will ensure failure if the expected exception is not raised.

I added a commit to the end of the PR to address this.

cdecker · 2017-12-13T12:13:19Z

tests/test_lightningd.py

+            self.fail("huge locktime ignored?")
+
+        # We don't have enough left to cover fees if we try to spend it all.
+        try:


Doesn't this implicitly assume an ordering of the internal checks, i.e., first we check the funding amounts and only then we check the locktimes? After all we didn't change the locktime blocks, but now expect a different exception. Spinning up a third node, with compatible locktimes to l1, and then connecting to l1 but with the wrong funding amount gets rid of this assumption.

The commit I added to the end of the PR now changes the max-locktime and restarts l2, which we'd be doing in the next step anyway.

Yep, much cleaner. Thanks!

cdecker · 2017-12-13T12:49:09Z

common/daemon_conn.h

+ * @dc: the daemon_conn to clean up.
+ *
+ * This is used by gossipd when a peer is handed back, and we no longer
+ * want to deal with it via daemon_conn.  @dc must not be used after this!


Could return NULL like tal_free so calls would look like this:

dc = daemon_conn_clear(dc)

to signal that the dc has been taken care of. The only call for daemon_conn_clear is directly followed by a tal_free on the daemon_conn anyway.

Hmm, yes, but as you say it's only got one caller, so maybe overkill.

rustyrussell · 2017-12-17T02:44:35Z

Thanks! Nice work!

cdecker added cleanup enhancement gossip labels Dec 8, 2017

cdecker added this to the v0.6 milestone Dec 8, 2017

cdecker requested a review from rustyrussell December 8, 2017 12:45

cdecker mentioned this pull request Dec 8, 2017

WIP: Announce channels to the involved peers directly after funding locked #414

Closed

rustyrussell mentioned this pull request Dec 11, 2017

Gossip improvements #430

Closed

jb55 mentioned this pull request Dec 12, 2017

Closing negotiations too restrictive #435

Closed

cdecker and others added 21 commits December 13, 2017 12:33

gossip: Remove HSM_FD from handshake

98b1a66

routing: Make routing_state aware of its own ID

37e9d14

This is used to identify our own announcements. Signed-off-by: Christian Decker <[email protected]>

opts: Change alias to be u8*, better matches the unicode nature

4c13032

We are still generating only char* style aliases, but the field is defined to be unicode, which doesn't mix too well with char. Signed-off-by: Christian Decker <[email protected]>

gossip: Passing alias, color and wireaddrs through to gossipd

7427c8a

First step towards `gossipd` managing the `node_announcement`. Signed-off-by: Christian Decker <[email protected]>

routing: Return boolean from handle_channel_announcement

abe9192

This will later be used to determine whether or not we should announce ourselves as a node. Signed-off-by: Christian Decker <[email protected]>

routing: Add local and sigfail to trace when receiving cannounce

dfc5a6e

Signed-off-by: Christian Decker <[email protected]>

gossip: Forward when we don't have a valid node_announcement yet

aa67236

Signed-off-by: Christian Decker <[email protected]>

routing: Do not set an empty channel_announcement if none is given

88218a0

Signed-off-by: Christian Decker <[email protected]>

json_fund_channel: give more details than "peer died".

c1b4f41

Rather than using the destructor, hook up the cmd so we can close it. peers are allocated off ld, so they are only destroyed explicitly. Signed-off-by: Rusty Russell <[email protected]>

wireaddr: marshal empty address properly.

6793efe

On unmarshal, we stop unmarshaling on a 0 (ADDR_TYPE_PADDING) type. So we should also stop marshaling in that case. Signed-off-by: Rusty Russell <[email protected]>

openingd: return to master for more gossip when negotiation fails.

139774c

We can open other channels, if we want. Signed-off-by: Rusty Russell <[email protected]>

openingd: handle ERROR packets (if other end fails negotiation).

c984fb2

Signed-off-by: Rusty Russell <[email protected]>

test_lightningd: add test for funding failures.

6889fe7

We should not disconnect from a peer just because it fails opening; we should return it to gossipd, and give a meaningful error. Closes: ElementsProject#401 Signed-off-by: Rusty Russell <[email protected]>

daemon_conn: helper to release daemon_conn.

616bc26

We'll want this for the next change, where gossipd migrates remote peers back to local ones. Signed-off-by: Rusty Russell <[email protected]>

Makefile: make gossipd objects depend correctly on its own headers.

62634e9

Signed-off-by: Rusty Russell <[email protected]>

subd: if a required daemon exits, wait instead of killing it.

c552fa8

Otherwise we always say it died because we killed it, so we don't get the exit status. Signed-off-by: Rusty Russell <[email protected]>

gossipd: don't hand length to route code, it's implied.

b9eddbe

Signed-off-by: Rusty Russell <[email protected]>

gossipd: don't increment broadcast_index until *after* message sent.

a4512ea

If the peer is moved from remote to local, this may be lost; it's more secure to increment after we've sent the broadcast.

rustyrussell and others added 3 commits December 13, 2017 12:36

pytest: Fix a flaky channel_reenable test

a22d341

It was relying on the message order instead of waiting the desired state. Signed-off-by: Christian Decker <[email protected]>

channel: Directly send announcements and updates to gossipd

c787e51

Signed-off-by: Christian Decker <[email protected]>

cdecker force-pushed the gossip-node-announce branch from 8674880 to c787e51 Compare December 13, 2017 11:36

pytest: Minor cleanup

234b412

Now using assertRaisesRegex instead of try-except and added restart to nodes. Signed-off-by: Christian Decker <[email protected]>

cdecker commented Dec 13, 2017

View reviewed changes

cdecker mentioned this pull request Dec 15, 2017

Let channeld add the outgoing channel as soon as funding locked on both sides #450

Merged

rustyrussell merged commit e154f4a into ElementsProject:master Dec 17, 2017

cdecker deleted the gossip-node-announce branch January 28, 2018 00:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let gossipd automatically announce the node when a local channel_announcement is queued #413

Let gossipd automatically announce the node when a local channel_announcement is queued #413

cdecker commented Dec 8, 2017

cdecker commented Dec 12, 2017

cdecker commented Dec 13, 2017

cdecker left a comment

cdecker Dec 13, 2017

rustyrussell Dec 17, 2017

cdecker Dec 13, 2017

cdecker Dec 13, 2017

cdecker Dec 13, 2017

cdecker Dec 13, 2017

cdecker Dec 13, 2017

rustyrussell Dec 17, 2017

cdecker Dec 13, 2017

rustyrussell Dec 17, 2017

rustyrussell commented Dec 17, 2017

Let gossipd automatically announce the node when a local channel_announcement is queued #413

Let gossipd automatically announce the node when a local channel_announcement is queued #413

Conversation

cdecker commented Dec 8, 2017

cdecker commented Dec 12, 2017

cdecker commented Dec 13, 2017

cdecker 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

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

rustyrussell commented Dec 17, 2017