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

multi: send a channel update with disabled flag set on channel close #1387

Merged
merged 6 commits into from Jul 24, 2018

Conversation

Projects
None yet
5 participants
@wpaulino
Collaborator

wpaulino commented Jun 14, 2018

Fixes #1132.

@wpaulino

This comment has been minimized.

Collaborator

wpaulino commented Jun 14, 2018

There's a flake on Travis that needs to be addressed first. Will ping once done.

@wpaulino

This comment has been minimized.

Collaborator

wpaulino commented Jun 15, 2018

Should be good for review now.

@cfromknecht

Nice! Will be good to get these changes in to help quell lingering routing failures while channels are closing. Did an initial pass to familiarize myself w/ the PR

server.go Outdated
pubKey := s.identityPriv.PubKey()
errChan := s.authGossiper.ProcessLocalAnnouncement(update, pubKey)
return <-errChan

This comment has been minimized.

@cfromknecht

cfromknecht Jun 29, 2018

Collaborator

select on quit as well

This comment has been minimized.

@wpaulino

wpaulino Jun 30, 2018

Collaborator

Fixed.

peer.go Outdated
// disableChannel disables a channel, resulting in it not being able to forward
// payments. This is done by sending a new channel update across the network
// with the disabled flag set.
func disableChannel(s *server) func(wire.OutPoint) error {

This comment has been minimized.

@cfromknecht

cfromknecht Jun 29, 2018

Collaborator

this currying can be avoided by just having the server as the receiver

func (s *server) disableChannel(op *wire.OutPoint) error

then can pass the function around as s.disableChannel

This comment has been minimized.

@wpaulino

wpaulino Jun 30, 2018

Collaborator

Fixed.

lnd_test.go Outdated
@@ -801,6 +801,9 @@ func checkChannelPolicy(policy, expectedPolicy *lnrpc.RoutingPolicy) error {
expectedPolicy.TimeLockDelta,
policy.TimeLockDelta)
}
if policy.Disabled != expectedPolicy.Disabled {
return errors.New("edge should be disable but isn't")

This comment has been minimized.

@cfromknecht

cfromknecht Jun 29, 2018

Collaborator

disabled -> disabled

This comment has been minimized.

@wpaulino

wpaulino Jun 30, 2018

Collaborator

Fixed.

@wpaulino wpaulino force-pushed the wpaulino:send-disable-chan-update branch from 356ebda to c3c6efb Jun 30, 2018

wpaulino added some commits Jun 14, 2018

test: wait until closed channel notification is received
In this commit, we modify the graph topology notifications test to wait
until a graph update notification has been received for a closed
channel, rather than assume the next graph update will be for the closed
channel itself.

@wpaulino wpaulino force-pushed the wpaulino:send-disable-chan-update branch from c3c6efb to 7bc9eb9 Jul 12, 2018

@halseth

I'm wondering if we can send out disable messages even more aggressively. Could we disable the channel every time it is removed from the switch (as this is the point it cannot route payments anymore), and enable it when it is added?

Not sure if this would lead to unnecessary spam on them network, but it would also help by disabling channels for offline nodes. Or would this be leak any sensitive information? (I don't think so, as it is fairly easy to ping a node to see if it's offline anyway).

Thoughts? cc @Roasbeef @cfromknecht

@@ -78,6 +78,10 @@ type chanCloseCfg struct {
// broadcastTx broadcasts the passed transaction to the network.
broadcastTx func(*wire.MsgTx) error
// disableChannel disables a channel, resulting in it not being able to
// forward payments.
disableChannel func(wire.OutPoint) error

This comment has been minimized.

@halseth

halseth Jul 12, 2018

Collaborator

why not just disable the channel in unregisterChannel?

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Jul 17, 2018

Not really a fan of disabling the channel each time a peer connects/disconnects. It leaks more information, and also can get a bit spammy if the peer is flapping for w/e reason.

@Roasbeef Roasbeef added this to the 0.5 milestone Jul 17, 2018

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Jul 23, 2018

Based on the recent convo in #1605, I'm warming up a bit more to sending it out each time something disconnects from the link itself, as it'll help to reduce routing failures over all. Also Johan brings up a good point that one can simply attempt to connect to nodes in order to get this type of reachability information.

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Jul 23, 2018

The one worry is the level of spam really that this can generate if a peer is very flappy for w/e reason. On our send, I think we can just enforce stricter controls on how often we send this out, in addition to the trickle logic already present in the gossiper itself.

@Roasbeef

LGTM 🥁

@Roasbeef Roasbeef merged commit e0baa49 into lightningnetwork:master Jul 24, 2018

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.2%) to 54.351%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wpaulino wpaulino deleted the wpaulino:send-disable-chan-update branch Jul 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment