Skip to content
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

discovery/gossiper: reliably send channel update msg to remote peer #1595

Merged

Conversation

@wpaulino
Copy link
Collaborator

commented Jul 20, 2018

In this commit, we address an issue where it's possible that we failed
to send the channel update message for a channel to the remote peer due
to them being offline. This would cause issues with private channels, as
a channel update message is needed for the remote peer to be aware of
the channel's forwarding policy. Due to the latest changes to
NotifyWhenOnline, we can now attempt to send the message every time we
detect the remote peer is online until success.

Depends on #1505.
Fixes #1347.

@wpaulino wpaulino changed the title Send channel update reliably discovery/gossiper: reliably send channel update msg to remote peer Jul 20, 2018

@wpaulino wpaulino assigned wpaulino and unassigned wpaulino Jul 20, 2018

@wpaulino wpaulino force-pushed the wpaulino:send-channel-update-reliably branch 2 times, most recently from b3f3aa9 to 23d81bd Jul 20, 2018

Show resolved Hide resolved discovery/gossiper.go Outdated
Show resolved Hide resolved discovery/gossiper.go Outdated
Show resolved Hide resolved discovery/gossiper.go Outdated
Show resolved Hide resolved discovery/gossiper.go Outdated

@Roasbeef Roasbeef added this to the 0.5 milestone Aug 1, 2018

@wpaulino wpaulino force-pushed the wpaulino:send-channel-update-reliably branch from 23d81bd to e64662d Aug 14, 2018

Show resolved Hide resolved discovery/gossiper.go Outdated
Show resolved Hide resolved discovery/gossiper.go Outdated

@Roasbeef Roasbeef modified the milestones: 0.5, 0.5.1 Aug 21, 2018

@wpaulino wpaulino force-pushed the wpaulino:send-channel-update-reliably branch from e64662d to 9749e00 Sep 7, 2018

Show resolved Hide resolved discovery/gossiper.go Outdated
Show resolved Hide resolved discovery/gossiper.go Outdated
Show resolved Hide resolved discovery/gossiper.go Outdated
Show resolved Hide resolved discovery/gossiper.go

@halseth halseth modified the milestones: 0.5.1, 0.5.2 Sep 20, 2018

@robtex

This comment has been minimized.

Copy link

commented Oct 9, 2018

Any updates on this? I experience this a lot since i tend to leave the house without waiting for X confirmations for my mobile wallet to establish a channel.
Workaround would be to permanently disable wifi or leave phone at home when i walk the dogs, but it feels a bit wrong.

@wpaulino

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 9, 2018

@robtex hoping to get this addressed within the next week or two.

@joostjager

This comment has been minimized.

Copy link
Collaborator

commented Feb 5, 2019

When we merge this, the downgrade path to 0.5.2 will be closed because of the db migration in this PR.

@wpaulino wpaulino force-pushed the wpaulino:send-channel-update-reliably branch 2 times, most recently from 1e2a0cb to 2d7bf4b Feb 6, 2019

@wpaulino

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 6, 2019

Broke up the reliable send logic into its own file. PTAL @cfromknecht @halseth.

@wpaulino wpaulino force-pushed the wpaulino:send-channel-update-reliably branch 2 times, most recently from 8e413a6 to b793f92 Feb 6, 2019

Show resolved Hide resolved discovery/message_store_test.go
Show resolved Hide resolved discovery/reliable_sender.go Outdated
Show resolved Hide resolved discovery/reliable_sender.go Outdated
Show resolved Hide resolved discovery/reliable_sender.go
Show resolved Hide resolved discovery/reliable_sender.go
Show resolved Hide resolved discovery/gossiper.go Outdated
Show resolved Hide resolved discovery/gossiper.go

@wpaulino wpaulino force-pushed the wpaulino:send-channel-update-reliably branch 2 times, most recently from ba04fbf to 0112d58 Feb 14, 2019

@wpaulino

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 14, 2019

Show resolved Hide resolved discovery/reliable_sender.go Outdated
Show resolved Hide resolved discovery/gossiper.go Outdated
Show resolved Hide resolved discovery/reliable_sender.go
Show resolved Hide resolved discovery/gossiper.go Outdated

@wpaulino wpaulino force-pushed the wpaulino:send-channel-update-reliably branch from 0112d58 to e3e0ec5 Feb 15, 2019

wpaulino added some commits Feb 6, 2019

discovery/message_store: add gossip message store
In this commit, we add a new store within the database that'll be
responsible for storing gossip messages which we need to reliably send
to peers. This aims to replace the current messageStore that exists
within the gossiper, so much of this logic is borrowed from there.
One of the main differences between the two is that we now index
messages with a new key format in which we take into account the
message's type. This allows us to store different messages for a
specific channel with a peer. The old key format is still supported in
order to prevent a database migration.
channeldb: add gossiper message store key migration
In this commit, we introduce a migration for the message store
sub-bucket that will migrate all keys within it to a new key format.
This new key format is composed of the peer's public key, followed by
the short channel ID, followed by the message type. This migration is
needed in order to provide backwards-compatibility with messages that
were previously stored before the introduction of the new key format.
server+discovery: remove channeldb.DB reference within the gossiper
Now that we've replaced the built-in messageStore with the
channeldb.GossipMessageStore, the reference to channeldb.DB is no longer
needed.
discovery/reliable_sender: implement message-agnostic reliable sender
In this commit, we implement a new subsystem for the gossiper that
uses some of the existing logic for resending channel announcement
signatures and implements it in a way to make it message-agnostic,
meaning that any type of message can be resent. Along the way we also
modify the way this works to prevent multiple goroutines per peer _and_
message.

A peerHandler will be spawned for each peer for which we attempt to send
a message reliably to. This handler is responsible for managing requests
to reliably send messages to a peer while also taking the peer's
connection lifecycle into account by requesting notifications for when
the peer connects/disconnects. A peer connection notification is first
requested to determine when we should attempt to send any pending
messages. After the messages are sent, a peer disconnection notification
is requested to ensure we don't continue to request connection
notifications while the peer remains connected. Once there are no more
pending messages left to be sent for a given peer, the peerHandler can
be torn down.
server+discovery: send channel updates to remote peers reliably
In this commit, we also allow channel updates for our channels to be
sent reliably to our channel counterparty. This is especially crucial
for private channels, since they're not announced, in order to ensure
each party can receive funds from the other side.

@wpaulino wpaulino force-pushed the wpaulino:send-channel-update-reliably branch from e3e0ec5 to 12168f0 Feb 15, 2019

@cfromknecht

This comment has been minimized.

Copy link
Collaborator

commented Feb 15, 2019

Tested the migration via dryrun on my mainnet node, no issues to report. Very clean and structured PR, LGTM 🔥

@Roasbeef

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

So do we want to add the --no-db-migrations option before we merge this in? I'm not sure how widely advertised it'll be since it won't be in any release notes, but I have no strong arguments against it. In a similar vein, we should also add a --no-commit-updates mode that allows lnd to come up, but ensure that it won't sign any commitment updates (maybe just disables all the channels), nor will it allow any sort of force close at all. This would make restoring from a "dirty" back up a little bit more save.

@halseth
Copy link
Collaborator

left a comment

This PR LGTM now ⚡️

// to assemble the full proof yet (maybe because of a crash),
// we will send them the full proof if we notice that they
// retry sending their half proof.
if chanInfo.AuthProof != nil {

This comment has been minimized.

Copy link
@halseth

halseth Feb 19, 2019

Collaborator

Check can be removed indeed. Not that it matters though...

}

// Otherwise, we'll attempt to stream the message to the handler.
// There's a subtle race condition where the handler can be torn down

This comment has been minimized.

Copy link
@halseth

halseth Feb 19, 2019

Collaborator

This is still a bit strange, but I guess it is okay 🙃 Current solution should work.

This comment has been minimized.

Copy link
@wpaulino

wpaulino Feb 19, 2019

Author Collaborator

I don't think there are any other ways around it. Do you have any suggestions?

@Roasbeef Roasbeef merged commit 2bf2261 into lightningnetwork:master Feb 20, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on send-channel-update-reliably at 59.902%
Details

High Priority automation moved this from Needs review to Done Feb 20, 2019

@wpaulino wpaulino deleted the wpaulino:send-channel-update-reliably branch Feb 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.