Skip to content

master: Move the gossipd initialization after the other inits#1440

Merged
cdecker merged 2 commits into
ElementsProject:masterfrom
cdecker:fix-reconnect-race
Apr 30, 2018
Merged

master: Move the gossipd initialization after the other inits#1440
cdecker merged 2 commits into
ElementsProject:masterfrom
cdecker:fix-reconnect-race

Conversation

@cdecker
Copy link
Copy Markdown
Member

@cdecker cdecker commented Apr 29, 2018

If we start accepting peer connections before we initialized some of the other
parts (mainly the chaintopology) we could end up asking for stuff that isn't
ready yet (blockchain head for example).

Fixes #1437

@cdecker cdecker self-assigned this Apr 29, 2018
@cdecker cdecker requested a review from rustyrussell April 29, 2018 17:04
If we start accepting peer connections before we initialized some of the other
parts (mainly the chaintopology) we could end up asking for stuff that isn't
ready yet (blockchain head for example).

Signed-off-by: Christian Decker <decker.christian@gmail.com>
@rustyrussell
Copy link
Copy Markdown
Contributor

This revealed a new race, however; activating peer means we tried sending to gossipd before it was ready.

Let's start gossipd, then explicitly tell it to activate. That also improves parallelism.

This means gossipd is live and we can tell it things, but it won't
receive incoming connections.  The split also means that the main daemon
continues (eg. loading peers from db) while gossipd is loading from the store,
potentially speeding startup.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Copy link
Copy Markdown
Contributor

@ZmnSCPxj ZmnSCPxj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 3c6a943

Affects #1323

@ZmnSCPxj
Copy link
Copy Markdown
Contributor

This is supposed to fix #1437? Yes/No/Maybe?

@rustyrussell
Copy link
Copy Markdown
Contributor

I think so, though @cdecker didn't reference a PR in the original?

@cdecker
Copy link
Copy Markdown
Member Author

cdecker commented Apr 30, 2018

Let's start gossipd, then explicitly tell it to activate. That also improves parallelism.

Ok, shall we defer the sending of gossipctl_init or do you prefer adding a new activate message that sets up the listeners?

EDIT: only saw your added commit now.

I think so, though @cdecker didn't reference a PR in the original?

It's a bug I encountered when restarting sleepyark, I didn't get a chance to go through the issue list. I'll edit the OP to include fixes markers.

@cdecker
Copy link
Copy Markdown
Member Author

cdecker commented Apr 30, 2018

ACK 3c6a943

@cdecker cdecker merged commit f083a69 into ElementsProject:master Apr 30, 2018
@sadilek sadilek mentioned this pull request Apr 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants