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

rpc: Add SubscribeChannels RPC. #1988

Merged

Conversation

@valentinewallace
Copy link
Collaborator

valentinewallace commented Sep 28, 2018

Closes #1235.

In this PR, we add a new streaming RPC call that allows clients to be notified whenever a channel becomes inactive, active, or closed. This is useful for any type of application that would otherwise need to poll the channel state to keep up to date on what channels are active, inactive, or closed.

In service of this goal, we add an additional subsystem called ChannelNotifier that notifies its clients upon any of the aforementioned channel state changes. It uses two hooks within the peer to know when channels become active/inactive.

For closed channel events, we add a proxy in the ChainArbitrator that proxies between all of its chainWatchers' closed channel notifications and pipes them into the ChannelNotifier.

@valentinewallace valentinewallace force-pushed the valentinewallace:subscribe-chans-rpc branch from c377871 to 999de48 Sep 28, 2018

@halseth
Copy link
Collaborator

halseth left a comment

Will be a very useful RPC to have! 😀

Show resolved Hide resolved rpcserver.go Outdated
Show resolved Hide resolved channotifier.go Outdated
Show resolved Hide resolved channotifier.go Outdated
Show resolved Hide resolved peer.go Outdated
Show resolved Hide resolved peer.go Outdated
Show resolved Hide resolved peer.go Outdated
Show resolved Hide resolved peer.go Outdated
Show resolved Hide resolved contractcourt/chain_arbitrator.go Outdated

@valentinewallace valentinewallace force-pushed the valentinewallace:subscribe-chans-rpc branch 3 times, most recently from 2925953 to 1cec722 Oct 22, 2018

Show resolved Hide resolved rpcserver.go Outdated
Show resolved Hide resolved rpcserver.go Outdated
Show resolved Hide resolved test_utils.go Outdated
Show resolved Hide resolved channelnotifier/channelnotifier.go Outdated
Show resolved Hide resolved channelnotifier/channelnotifier.go Outdated
Show resolved Hide resolved channelnotifier/channelnotifier.go Outdated
Show resolved Hide resolved channelnotifier/channelnotifier.go Outdated
Show resolved Hide resolved contractcourt/chain_arbitrator.go Outdated
server.go Outdated
},
NotifyInactiveChannelEvent: func(chanID lnwire.ShortChannelID) {
select {
case s.channelNotifier.NewInactiveChannels.ChanIn() <- chanID:

This comment has been minimized.

Copy link
@halseth

halseth Oct 25, 2018

Collaborator

To make sure is always non-blocking, I'm wondering if we should select on default here? Or somehow determine that the queue is active somehow.

Need to think about this abit.

This comment has been minimized.

Copy link
@valentinewallace

valentinewallace Oct 27, 2018

Author Collaborator

let me know if you like my solution to this!

Show resolved Hide resolved lnrpc/rpc.proto Outdated

@valentinewallace valentinewallace force-pushed the valentinewallace:subscribe-chans-rpc branch 2 times, most recently from c827999 to 6ff6b30 Oct 27, 2018

func (d *DB) FetchOpenChannel(
nodeID *btcec.PublicKey, chanID lnwire.ShortChannelID) (*OpenChannel, error) {

channels, err := d.FetchOpenChannels(nodeID)

This comment has been minimized.

Copy link
@halseth

halseth Oct 29, 2018

Collaborator

Instead this should be done by fetching the channel directly from the bucket, instead of iterating over all the node's channels. Not sure if this is compatible with the shortChanID, so might have to pass in the chanPoint instead,

This comment has been minimized.

Copy link
@valentinewallace

valentinewallace Oct 30, 2018

Author Collaborator

fixed, we still iterate over the chainhash buckets for a node but I think that's acceptable?

@valentinewallace valentinewallace force-pushed the valentinewallace:subscribe-chans-rpc branch 4 times, most recently from 7f658af to 0029656 Oct 30, 2018

Show resolved Hide resolved channeldb/db.go Outdated
Show resolved Hide resolved channeldb/db.go Outdated
Show resolved Hide resolved channeldb/db.go Outdated
Show resolved Hide resolved channeldb/db.go Outdated
Show resolved Hide resolved channelnotifier/channelnotifier.go Outdated
Show resolved Hide resolved lnd_test.go Outdated
Show resolved Hide resolved rpcserver.go
Show resolved Hide resolved rpcserver.go Outdated
Show resolved Hide resolved rpcserver.go Outdated
Show resolved Hide resolved htlcswitch/link.go

@valentinewallace valentinewallace force-pushed the valentinewallace:subscribe-chans-rpc branch from 0029656 to 3b7bea8 Nov 2, 2018

@cfromknecht
Copy link
Collaborator

cfromknecht left a comment

Great work @valentinewallace, looking forward to having these changes in master! they should help simplify not only our application interface in removing long-polling, but also as hooks for our integration tests in general. will likely result in less WaitPredicates over time.

my main question for you and @halseth is if we want to distinguish notifying on pending and live channels, as currently this treats them equivalently. i imagine it would be nice as a subscriber to know whether the channel is truly active and ready to accept payments/forwards. would probably result in one extra notification type, and a little extra code in the switch to handle pending/live separately, and also to notify when UpdateShortChanIDs is called on pending links.

Thoughts?

Show resolved Hide resolved rpcserver.go Outdated
Show resolved Hide resolved rpcserver.go
Show resolved Hide resolved rpcserver.go Outdated
Show resolved Hide resolved subscribe/subscribe.go
Show resolved Hide resolved channelnotifier/channelnotifier.go
Show resolved Hide resolved htlcswitch/switch.go Outdated
Show resolved Hide resolved htlcswitch/switch.go Outdated
Show resolved Hide resolved channeldb/channel_test.go Outdated
Show resolved Hide resolved channeldb/channel_test.go Outdated
Show resolved Hide resolved lnd_test.go

@valentinewallace valentinewallace force-pushed the valentinewallace:subscribe-chans-rpc branch from 3b7bea8 to d897cc7 Nov 5, 2018

@halseth
Copy link
Collaborator

halseth left a comment

Agree that it is useful to distinguish notifying on pending and live channels 👍

Show resolved Hide resolved rpcserver.go
Show resolved Hide resolved channeldb/channel_test.go Outdated
Show resolved Hide resolved channeldb/channel_test.go Outdated
Show resolved Hide resolved lnd_test.go Outdated
Show resolved Hide resolved lnd_test.go Outdated

// Since the link addition has succeeded, this channel is now officially
// active and we can tell the ChannelNotifier about it.
channel, err := s.cfg.DB.FetchOpenChannel(peerPub[:], link.ChannelPoint())

This comment has been minimized.

Copy link
@halseth

halseth Nov 5, 2018

Collaborator

Not sure if there are performance considerations we should take doing DB queries when adding and removing links. Will let @cfromknecht comment on this . :)

@valentinewallace valentinewallace force-pushed the valentinewallace:subscribe-chans-rpc branch 4 times, most recently from 47cf860 to bf5b754 Nov 9, 2018

@Roasbeef Roasbeef added this to the 0.6 milestone Jan 16, 2019

@valentinewallace valentinewallace force-pushed the valentinewallace:subscribe-chans-rpc branch 5 times, most recently from f98933a to 2012143 Jan 17, 2019

@valentinewallace valentinewallace force-pushed the valentinewallace:subscribe-chans-rpc branch from 2012143 to 71f61f2 Jan 25, 2019

peer.go Outdated
@@ -1693,6 +1693,11 @@ out:
continue
}

// Inform the ChannelNotifier that this channel has transitioned
// from pending open to open.
p.server.channelNotifier.NotifyOpenChannelEvent(newChan.IdentityPub,

This comment has been minimized.

Copy link
@halseth

halseth Jan 30, 2019

Collaborator

Would a better place for this be in the fundingmanager? If we want this to notify about the channel being "marked open" in the database, then it could be done here:

if err := completeChan.MarkAsOpen(shortChanID); err != nil {

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Feb 1, 2019

Member

Agreed would be nice to move to the fundingManager. Would also get rid of another instance of usage of the server's "God" pointer.

This comment has been minimized.

Copy link
@valentinewallace

valentinewallace Feb 6, 2019

Author Collaborator

fixed :)

@valentinewallace valentinewallace force-pushed the valentinewallace:subscribe-chans-rpc branch 3 times, most recently from b307d35 to fbf638e Feb 2, 2019

@@ -298,6 +298,88 @@ func fileExists(path string) bool {
return true
}

// FetchOpenChannel starts a new database transaction and returns the stored

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Feb 6, 2019

Member

This is no longer needed, now that we have: https://github.com/lightningnetwork/lnd/blob/master/channeldb/db.go#L412

The added tests can go also.

This comment has been minimized.

Copy link
@valentinewallace

valentinewallace Feb 6, 2019

Author Collaborator

fixed

@valentinewallace valentinewallace force-pushed the valentinewallace:subscribe-chans-rpc branch 2 times, most recently from 8da194a to 5f059ab Feb 6, 2019

valentinewallace and others added some commits Sep 27, 2018

rpcserver: add helper functions to format open and closed RPC channels.
`createRPCChannel` is used by the `listchannels` RPC call and will be
used by `subscribechannels` as well.
`createRPCClosedChannel` is used by the `closedchannels` RPC call and
will also be used by `subscribechannels`.

This is used by the `listchannels` RPC call and will be used by
`subscribechannels` as well. Its purpose is to mitigate code duplication
between the two RPC calls.
subscribe: add new subscribe package
This commit creates a new package 'subscribe', that exposes a common
Client-Server subscription system, that can be shared among packages.
lnd: introduce the ChannelNotifier.
This commit introduces the channel notifier which is a central source
of active, inactive, and closed channel events.

This notifier was originally intended to be used by the `SubscribeChannels`
streaming RPC call, but can be used by any subsystem that needs to be
notified on a channel becoming active, inactive or closed.

It may also be extended in the future to support other types of notifications.
htlcswitch/link: add ChannelPoint() to retrieve the channel outpoint.
This function will be used in the switch to retrieve the channel point for a link,
allowing the switch to retrieve individual channels from the database.
lnd_test: alter basic channel creation test to test chan subscription.
Because the integration tests are already long-running, it is preferable to
add testing for the RPC channel update subscription to an existing test rather
than adding additional tests.

@valentinewallace valentinewallace force-pushed the valentinewallace:subscribe-chans-rpc branch from 5f059ab to b826101 Feb 6, 2019

@Roasbeef
Copy link
Member

Roasbeef left a comment

LGTM 🐲

@Roasbeef Roasbeef merged commit c05a7c5 into lightningnetwork:master Feb 6, 2019

1 of 2 checks passed

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

High Priority automation moved this from Needs review to Done Feb 6, 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.