Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 20, 2017

Conversation

cdecker
Copy link
Member

@cdecker cdecker commented Dec 15, 2017

This is an alternative to #414. The main difference is that instead of having unsigned channel_announcement and channel_update messages that are allowed through if they are local, we add a new message that channeld can send to gossipd when both sides lock. This message now contains only the necessary information for the outgoing direction, and will immediately activate the channel so we can use it.

Fixes #284
Depends on #413 (only the last 4 commits are for this PR, all others are from #413)

@cdecker cdecker added the gossip label Dec 15, 2017
@cdecker cdecker added this to the v0.6 milestone Dec 15, 2017
@cdecker cdecker requested a review from rustyrussell December 15, 2017 23:45
@cdecker
Copy link
Member Author

cdecker commented Dec 17, 2017

Rebased on top of master now that #413 is merged.

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Minor weirdness needs deleting, otherwise great!

gossipd/gossip.c Outdated

/* Sent from channeld through its gossip_fd */
case WIRE_GOSSIP_LOCAL_ADD_CHANNEL:
break;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is deeply confusing.

Just remove gossip_local_add_channel and the weird call to it: the "break;" here does exactly what we want if the master sends us that message for some reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

See also line 1601, and the weird function it calls, which AFAICT shouldn't exist?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh wow, seems I botched up a rebase somewhere, sorry about that

@cdecker
Copy link
Member Author

cdecker commented Dec 18, 2017

Great, thanks for the review @rustyrussell, I added a fixup and I'll squash and merge if you're ok with it

@cdecker cdecker dismissed rustyrussell’s stale review December 19, 2017 13:42

Removed the scaffolding I completely forgot about :-)

Couldn't find a good place to put these messages, we probably want to
do the same capability based request routing that we did for the HSM,
but for now this just defines the message in the master messages file.

Signed-off-by: Christian Decker <[email protected]>
This adds the channel from us to the remote node and activates it with
our local parameters.

Signed-off-by: Christian Decker <[email protected]>
Relatively simple: until we reach funding-depth the channels should be
known locally, so we can already route through them, but they should
not be announced to peers to which the connection is non-local.

Signed-off-by: Christian Decker <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants