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: implement new query based channel graph synchronization #1106

Merged
merged 25 commits into from Jun 1, 2018

Conversation

Projects
None yet
4 participants
@Roasbeef
Member

Roasbeef commented Apr 17, 2018

In this commit, we implement the new gossip query features recently added to the specification. With this implemented, once most peers are updated, we can skip the extremely wasteful initial routing table dump on initial connect. We also add a new command line flag that allows nodes to opt out of real time channel updates all together. This is desirable, as passive routing nodes don't really need to be receiving any of the updates, and can save bandwidth by not receiving them at all.

One follow up to this PR we might want to consider (for greater savings) is to ensure that we only engage in a single channel graph sync outstanding. This would allow use to fully sync up using a single peer, then diff our state against the next peer using the new information gained, repeating until we think we're synced. This would only apply for the first few (3 or so) peers that we connect to though.

NOTE: This PR introduces a new database migration to populate the new indexes added for existing node deployments.

lnwire

In this PR, we add recognition of the data loss protected feature bit. We already implement the full feature set, but then never added the bit to our set of known features. Setting this will allow all the eclair mobile nodes on the network to recover their settled channel balances in the case of partial data loss.

Additionally, we both bits for the new gossip query features along with defining all the new message types. Note that at this point, we haven't yet added the zlib short channel ID compression. We'll add that in a follow up PR to not allow this PR to swell anymore.

discovery

We, introduce a new struct, the gossipSyncer. The role of this struct is to encapsulate the state machine required to implement the new gossip query range feature recently added to the spec. With this change, each peer that knows of this new feature will have a new goroutine that will be managed by the gossiper.

Once created and started, the gossipSyncer will start to progress through each possible state, finally ending at the chansSynced stage. In this stage, it has synchronized state with the remote peer, and is simply awaiting any new messages from the gossiper to send directly to the peer. Each message will only be sent if the remote peer actually has a set update horizon, and the message isn't before or after that horizon. A set of unit tests has been added to ensure that two state machines properly terminate and synchronize channel state.

The gossip now has complete knowledge of the current set of peers we're connected to that support the new range queries. Upon initial connect, InitSyncState will be called by the server if the new peer understands the set of gossip queries. This will then create a new spot in the peerSyncers map for the new syncer. For each new gossip query message, we'll then attempt to dispatch the message directly to the gossip syncer. When the peer has disconnected, we then expect the server to call the PruneSyncState method which will allow us to free up the resources.

Finally, when we go to broadcast messages, we'll send the messages directly to the peers that have gossipSyncer instances active, so they can properly be filtered out. For those that don't we'll broadcast directly, ensuring we skip all peers that have an active gossip syncer.

channeldb

We add a series of methods, and a new database index that we'll use to implement the new discovery.ChannelGraphTimeSeries interface interface. The primary change is that we now maintain two new indexes tracking the last update time for each node, and the last update time for each edge. These two indexes allow us to implement the NodeUpdatesInHorizon and ChanUpdatesInHorizon methods. The remaining methods added simply utilize the existing database indexes to allow us to respond to any peer gossip range queries.

We also add a new database migration required to update old database to the version of the database that tracks the update index for the nodes and edge policies. The migration is straight forward, we
simply need to populate the new indexes for the all the nodes, and then all the edges.

server

Finally, the server has been updated to signal the new feature bits that we understand, and also to decide if we should create a new gossip syncer, or just execute a full table dump upon initial connect.

Fixes #910

)
// ErrUnknownShortChanIDEncoding is a parametrized error that indicates that we
// came across an unkonw short channel ID encoding, and therefore were unable

This comment has been minimized.

@halseth

halseth Apr 17, 2018

Collaborator

nit: s/unkonw/unknown

This comment has been minimized.

@Roasbeef

Roasbeef Apr 25, 2018

Member

Fixed.

}
// synchronizeChanIDs is called by the channelGraphSyncer when we need to query
// the remote peer for its known set of channel ID"s within a particular block

This comment has been minimized.

@halseth

halseth Apr 17, 2018

Collaborator

nit: IDs

This comment has been minimized.

@Roasbeef

Roasbeef Apr 25, 2018

Member

Fixed.

case msgs := <-msgChan:
if len(msgs) != 3 {
t.Fatalf("expected 2 messages instead got %v "+

This comment has been minimized.

@halseth

halseth Apr 17, 2018

Collaborator

expected 3

This comment has been minimized.

@Roasbeef

Roasbeef Apr 25, 2018

Member

Fixed.

@@ -1596,7 +1754,7 @@ func (d *AuthenticatedGossiper) processNetworkAnnouncement(nMsg *networkMsg) []n
// We'll ignore any channel announcements that target any chain
// other than the set of chains we know of.
if !bytes.Equal(msg.ChainHash[:], d.cfg.ChainHash[:]) {
log.Errorf("Ignoring ChannelUpdate from "+
log.Error("Ignoring ChannelUpdate from "+

This comment has been minimized.

@halseth

halseth Apr 17, 2018

Collaborator

intended change?

This comment has been minimized.

@Roasbeef

Roasbeef Apr 25, 2018

Member

Nope, reverted!

func (c *ChannelGraph) ChanUpdatesInHorizon(startTime, endTime time.Time) ([]ChannelEdge, error) {
var edgesInHorizon []ChannelEdge
err := c.db.View(func(tx *bolt.Tx) error {

This comment has been minimized.

@merehap

merehap Apr 17, 2018

Contributor

Could you break this up into helper functions? IMO, ideally most functions should fit on a single screen or else it becomes difficult to hold the whole thing in one's head at the same time. Also unit testing becomes much easier.

I realize that edgesInHorizon is updated deep down, but I think that could be propagated up through the helper functions without much overhead.

I'm imagining that this entire anonymous function could be pulled out such that you end up with something like

err := c.db.View(func(tx *bolt.Tx) error {
    edgesInHorizon, pErr = processEdges(tx)
    return pErr
}

and then within processEdges (or whatever you want to call it) you'd extract the inner portion of the for loop into a processEdge function.

What do you think? If this is considered useful, there are a few other places in this PR were it applies.

This comment has been minimized.

@Roasbeef

Roasbeef Apr 25, 2018

Member

I'd say yes, if the processing function were used in other locations in the package/file. Atm, they aren't. The general code style in the project is to favor a clear control flow over excessive function modularization. In this case, you can read a single function and grok the logic rather than following around several other newly introduced functions.

Not sure what screen you use for viewing code, but it fits on mine ;)

// We'll run through the set of chanIDs and collate only the
// set of channel that are unable to be found within our db.
var cidBytes [8]byte

This comment has been minimized.

@merehap

merehap Apr 17, 2018

Contributor

This constant of "8" keeps showing up. Might want to create a constant for it to make it more clear what it is.

This comment has been minimized.

@Roasbeef

Roasbeef Apr 25, 2018

Member

I started to add a constant everywhere, but IMO it started to get in the way of the control flow throughout the file. 8 is the size of an unsigned integer. In each of the locations, the variable names and comments itself are enough context to understand that 8 is the size of an unsigned integer.

if edgeBytes := edges.Get(edgeKey[:]); edgeBytes != nil {
// In order to delete the old entry, we'll need to obtain the
// *prior* update time in order to delete it.
updateEnd := 33 + (8 * 3) + 2 + 1

This comment has been minimized.

@merehap

merehap Apr 17, 2018

Contributor

I think you definitely want a constant for this value and a comment explaining what's going on. Maybe sub-constants too.

This comment has been minimized.

@Roasbeef

Roasbeef Apr 25, 2018

Member

Went with adding additional detail to the constant here. Decided against a constant as it's only used in a single location in the file atm.

// query and are waiting for the final response from the remote
// peer before we perform a diff to see with channels they know
// of that we don't.
case waitingQueryRangeReply:

This comment has been minimized.

@merehap

merehap Apr 17, 2018

Contributor

Could you use helper functions to reduce the amount of code that is in these case statements? Basically I'm imagining that for each top-level case, there would be one helper function that would contain almost all of the code this is currently contained in the case.

This would

  • make channelGraphSyncer considerably shorter (easier to fit into one's head)
  • create a clear separation between high-level details and low-level details (which would be contained in the helper functions)
  • make unit testing way easier, cleaner, and more granular
  • decrease vertical line noise by decreasing line-wrapping for over-indentation

Note that the helper functions will probably need multiple return values to allow for the "return", "continue", and standard case endings.

This comment has been minimized.

@Roasbeef

Roasbeef Apr 25, 2018

Member

All of "transition" logic is already located in distinct functions. The logic in each of the case statements simply calls out to those, and handles the redundant case in each state. As is, the entire file already have a comprehensive set of unit tests for both the transition functions, and also the control flow interaction between two syncer instances.

Generally, in the codebase, we favor a clear control flow over excessive modularization. Vertical space isn't as much of an issue following this philosophy.

// We'll give a few hours room in our update
// horizon to ensure we don't miss any newer
// itesm.

This comment has been minimized.

@merehap

merehap Apr 17, 2018

Contributor

items*

@Roasbeef Roasbeef requested a review from aakselrod Apr 23, 2018

@halseth

Nits mostly, otherwise utACK 👍

// this interface to determine if we're already in sync, or need to request
// some new information from them.
type ChannelGraphTimeSeries interface {
// HighestChanID should return is the channel ID of the channel we know

This comment has been minimized.

@halseth

halseth Apr 25, 2018

Collaborator

nit: is

This comment has been minimized.

@Roasbeef

Roasbeef Apr 25, 2018

Member

Fixed.

// HighestChanID should return is the channel ID of the channel we know
// of that's furthest in the target chain. This channel will have a
// block height that's close to the current tip of the main chain as we
// know it. We'll use this to start our QueryChannelRange dance with

This comment has been minimized.

@halseth

halseth Apr 25, 2018

Collaborator

🕺

// remote peer's QueryChannelRange message.
FilterChannelRange(chain chainhash.Hash,
startHeight, endHeight uint32) ([]lnwire.ShortChannelID, error)
// FetchChanAnns returns a full set of channel announcements as well as

This comment has been minimized.

@halseth

halseth Apr 25, 2018

Collaborator

missing newline

This comment has been minimized.

@Roasbeef

Roasbeef Apr 25, 2018

Member

Fixed.

}
// Start starts the gossipSyncer and any goroutines that it needs to carry out
// it duties.

This comment has been minimized.

@halseth

halseth Apr 25, 2018

Collaborator

nit: its

This comment has been minimized.

@Roasbeef

Roasbeef Apr 25, 2018

Member

Fixed.

log.Debugf("Starting gossipSyncer(%x)", g.peerPub[:])
g.wg.Add(1)

This comment has been minimized.

@halseth

halseth Apr 25, 2018

Collaborator

nit: extra newline

This comment has been minimized.

@Roasbeef

Roasbeef Apr 25, 2018

Member

Fixed.

"and %v blocks after", g.peerPub[:], startHeight,
math.MaxUint32-startHeight)
// Finally, we'll craft the channel range query, using out starting

This comment has been minimized.

@halseth

halseth Apr 25, 2018

Collaborator

nit: s/out/our

This comment has been minimized.

@Roasbeef

Roasbeef Apr 25, 2018

Member

Fixed.

@@ -480,6 +501,15 @@ type msgWithSenders struct {
senders map[routing.Vertex]struct{}
}
// mergeSyncerMap is used to merge the set of senders of a particular message
// with peers that we have an active gossipSyncer with. We do this to ensure
// that we don't broadcast messages to any peers that

This comment has been minimized.

@halseth

halseth Apr 25, 2018

Collaborator

unfinished comment

This comment has been minimized.

@Roasbeef

Roasbeef Apr 25, 2018

Member

Fixed.

return nil, err
}
for _, channel := range chansInHorizon {
if channel.Info.AuthProof == nil {

This comment has been minimized.

@halseth

halseth Apr 25, 2018

Collaborator

add a comment to this check

This comment has been minimized.

@Roasbeef

Roasbeef Apr 25, 2018

Member

Fixed.

@@ -202,6 +202,8 @@ type config struct {
Color string `long:"color" description:"The color of the node in hex format (i.e. '#3399FF'). Used to customize node appearance in intelligence services"`
MinChanSize int64 `long:"minchansize" description:"The smallest channel size (in satoshis) that we should accept. Incoming channels smaller than this will be rejected"`
NoChanUpdates bool `long:"nochanupdates" description:"If specified, lnd will not request real-time channel updates from connected peers. This option should be used by routing nodes to save bandwidth."`

This comment has been minimized.

@halseth

halseth Apr 25, 2018

Collaborator

Since there might be routing nodes that wish to send active payments, I think this should instead say "nodes not not wishing to pay invoices" or similar.

This comment has been minimized.

@Roasbeef

Roasbeef Apr 25, 2018

Member

Well they can still send payments, they make just incur an extra routing attempt if they have a stale channel update.

@Roasbeef Roasbeef force-pushed the Roasbeef:new-graph-sync branch 3 times, most recently from 49b9b1e to b86e621 Apr 25, 2018

@cfromknecht

Have not had a chance to test, but gave a pretty thorough review and seems solid. Like halseth, just a couple nits. Fantastic work! 🔥

return "waitingQueryChanReply"
case chansSynced:
return "syncingChans"

This comment has been minimized.

@cfromknecht

cfromknecht Apr 26, 2018

Collaborator

s/synchingChans/chansSynced/

}
// FilterChannelRange returns the set of channels that we created between the
// start height and the end height. We'll use this to to a remote peer's

This comment has been minimized.

@cfromknecht

cfromknecht Apr 26, 2018

Collaborator

to to -> to respond to

if err != nil {
return nil, err
}
updates = append(updates, &lnwire.NodeAnnouncement{

This comment has been minimized.

@cfromknecht

cfromknecht Apr 26, 2018

Collaborator

this section looks similar to makeNodeAnn below, is there a reason this doesn't use it? maybe has to do with alias err handling?

cursor := edgeIndex.Cursor()
// We'll now iterate through the database, and find each
// channel ID that redoes within the specified range.

This comment has been minimized.

@cfromknecht

cfromknecht Apr 26, 2018

Collaborator

redoes?

// channel ID that redoes within the specified range.
var cid uint64
for k, _ := cursor.Seek(chanIDStart[:]); k != nil &&
bytes.Compare(k, chanIDEnd[:]) <= 0; k, _ = cursor.Next() {

This comment has been minimized.

@cfromknecht

cfromknecht Apr 26, 2018

Collaborator

Is this scan intended to be inclusive or exclusive wrt. to the end block? If inclusive, should we set max values for TxIndex and TxPosition in chanIDEnd? It seems the current behavior is exclusive, though the predicate could include the end block's coinbase txn (I think?).

},
})
copy(syncer.peerPub[:], peer.SerializeCompressed())
d.peerSyncers[routing.NewVertex(peer)] = syncer

This comment has been minimized.

@cfromknecht

cfromknecht Apr 26, 2018

Collaborator

Do we need to check if we already have a syncer? This is spawned in a go routine, so maybe not a bad idea

}
// Otherwise, it's the remote peer performing a
// query, which we'll attempt to deploy to.

This comment has been minimized.

@cfromknecht

cfromknecht Apr 26, 2018

Collaborator

deploy -> reply?

@Roasbeef Roasbeef force-pushed the Roasbeef:new-graph-sync branch 2 times, most recently from 4275976 to a9981a0 Apr 26, 2018

@Roasbeef Roasbeef added this to the 0.5 milestone May 2, 2018

Roasbeef added some commits Apr 17, 2018

lnwire: add recognition of the data loss proected feature bit
In this commit, we add recognition of the data loss protected feature
bit. We already implement the full feature set, but then never added the
bit to our set of known features.
discovery: add new gossipSyncer struct to manage sync state for each …
…peer

In this commit, introduce a new struct, the gossipSyncer. The role of
this struct is to encapsulate the state machine required to implement
the new gossip query range feature recently added to the spec. With this
change, each peer that knows of this new feature will have a new
goroutine that will be managed by the gossiper.

Once created and started, the gossipSyncer will start to progress
through each possible state, finally ending at the chansSynced stage. In
this stage, it has synchronized state with the remote peer, and is
simply awaiting any new messages from the gossiper to send directly to
the peer. Each message will only be sent if the remote peer actually has
a set update horizon, and the message isn't before or after that
horizon.

A set of unit tests has been added to ensure that two state machines
properly terminate and synchronize channel state.
discovery: update AuthenticatedGossiper to be aware of new gossipSyncers
In this commit, we update the logic in the AuthenticatedGossiper to
ensure that can properly create, manage, and dispatch messages to any
gossipSyncer instances created by the server.

With this set of changes, the gossip now has complete knowledge of the
current set of peers we're conneted to that support the new range
queries. Upon initial connect, InitSyncState will be called by the
server if the new peer understands the set of gossip queries. This will
then create a new spot in the peerSyncers map for the new syncer. For
each new gossip query message, we'll then attempt to dispatch the
message directly to the gossip syncer. When the peer has disconnected,
we then expect the server to call the PruneSyncState method which will
allow us to free up the resources.

Finally, when we go to broadcast messages, we'll send the messages
directly to the peers that have gossipSyncer instances active, so they
can properly be filtered out. For those that don't we'll broadcast
directly, ensuring we skip *all* peers that have an active gossip
syncer.
channeldb: add new methods required to implement new discovery.Channe…
…lGraphTimeSeries interface

In this commit, we add a series of methods, and a new database index
that we'll use to implement the new discovery.ChannelGraphTimeSeries
interface interface. The primary change is that we now maintain two new
indexes tracking the last update time for each node, and the last update
time for each edge. These two indexes allow us to implement the
NodeUpdatesInHorizon and ChanUpdatesInHorizon methods. The remaining
methods added simply utilize the existing database indexes to allow us to
respond to any peer gossip range queries.

A set of new unit tests has been added to exercise the added logic.
channeldb: add database migration for new node+edge update indexes
In this commit, we add a new database migration required to update old
database to the version of the database that tracks the update index for
the nodes and edge policies. The migration is straight forward, we
simply need to populate the new indexes for the all the nodes, and then
all the edges.
discovery+lnd: create new chanSeries impl of the ChannelGraphTimeSeri…
…es interface

In this commit, we create a new concrete implementation for the new
discovery.ChannelGraphTimeSeries interface. We also export the
createChannelAnnouncement method to allow the chanSeries struct to
re-use the existing code for creating wire messages from the database
structs.

Roasbeef added some commits Apr 17, 2018

config: add new command line option to disable chan update all together
In this commit, we add a new command line option to allow (ideally
routing nodes) to disable receiving up-to-date channel updates all
together. This may be desired as it'll allow routing nodes to save on
bandwidth as they don't need the channel updates to passively forward
HTLCs. In the scenario that they _do_ want to update their routing
policies, the first failed HTLC due to policy inconsistency will then
allow the routing node to propagate the new update to potential nodes
trying to route through it.
channeldb: ensure that when we delete a channel we delete entry in ed…
…ge update index

In this commit, we ensure that all indexes for a particular channel have
any relevant keys deleted once a channel is removed from the database.
Before this commit, if we pruned a channel due to closing, then its
entry in the channel update index would ever be removed.
discovery: add new SyncState method to gossipSyncer
This new method allows outside callers to sample the current state of
the gossipSyncer in a concurrent-safe manner. In order to achieve this,
we now only modify the g.state variable atomically.
discovery: attempt to request the full chan ann for stray chan updates
In this commit, we extend the AuthenticatedGossiper to take advantage of
the new query features in the case that it gets a channel update w/o
first receiving the full channel announcement. If this happens, we'll
attempt to find a syncer that's fully synced, and request the channel
announcement from it.

@Roasbeef Roasbeef force-pushed the Roasbeef:new-graph-sync branch from b07023f to 994d9cf May 31, 2018

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented May 31, 2018

Just pushed out a new version rebase on top of the current master (no conflicts, yay!). Will do some local testing before merging through to master.

It seems the eclair on mobile will no longer connect to nodes that don't have the data loss protect bit on. We've supported this for some time, but never actually advertised the feature bit. A commit in this PR fixes that so connectivity will be restored once it's in.

discovery: if unable to find gossipSyncer for peer, create one
In this commit we fix an existing bug caused by a scheduling race
condition. We'll now ensure that if we get a gossip message from a peer
before we create an instance for it, then we create one on the spot so
we can service the message. Before this commit, we would drop the first
message, and therefore never sync up with the peer at all, causing them
to miss channel announcements.

@Roasbeef Roasbeef merged commit da72f8a into lightningnetwork:master Jun 1, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 54.343%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment