From 4a755435e6b8b2d70cdb6d24164ce9f62b50e66d Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Tue, 9 Apr 2019 17:11:58 -0700 Subject: [PATCH 1/2] channeldb/graph: skip unknown edges in FetchChanInfos This commit modifies FetchChanInfos to skip any channels that are not in the graph at the time of the call. Currently the entire call will fail if the edge is not found, which stalls a gossip sync in the following scenario: 1. Remote peer queries for a channel range 2. We return the set of channel ids in that range 3. A channel from that set is removed from the graph, e.g. via close. 4. Remote peer queries for removed edge, causing the query to fail. To remedy this, we will now skip any edges that are not known in the database at the time of the query. This prevents the syncer state machines from halting, which otherwise could only be resolved by disconnecting and reconnecting. --- channeldb/graph.go | 17 ++++++++++++----- channeldb/graph_test.go | 18 ++++++++++++++++++ 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/channeldb/graph.go b/channeldb/graph.go index 68d5b87c9..fef94919f 100644 --- a/channeldb/graph.go +++ b/channeldb/graph.go @@ -1636,9 +1636,11 @@ func (c *ChannelGraph) FilterChannelRange(startHeight, endHeight uint32) ([]uint return chanIDs, nil } -// FetchChanInfos returns the set of channel edges that correspond to the -// passed channel ID's. This can be used to respond to peer queries that are -// seeking to fill in gaps in their view of the channel graph. +// FetchChanInfos returns the set of channel edges that correspond to the passed +// channel ID's. If an edge is the query is unknown to the database, it will +// skipped and the result will contain only those edges that exist at the time +// of the query. This can be used to respond to peer queries that are seeking to +// fill in gaps in their view of the channel graph. func (c *ChannelGraph) FetchChanInfos(chanIDs []uint64) ([]ChannelEdge, error) { // TODO(roasbeef): sort cids? @@ -1664,11 +1666,16 @@ func (c *ChannelGraph) FetchChanInfos(chanIDs []uint64) ([]ChannelEdge, error) { for _, cid := range chanIDs { byteOrder.PutUint64(cidBytes[:], cid) - // First, we'll fetch the static edge information. + // First, we'll fetch the static edge information. If + // the edge is unknown, we will skip the edge and + // continue gathering all known edges. edgeInfo, err := fetchChanEdgeInfo( edgeIndex, cidBytes[:], ) - if err != nil { + switch { + case err == ErrEdgeNotFound: + continue + case err != nil: return err } edgeInfo.db = c.db diff --git a/channeldb/graph_test.go b/channeldb/graph_test.go index c2ae2ab2c..12ff6a1b8 100644 --- a/channeldb/graph_test.go +++ b/channeldb/graph_test.go @@ -2006,6 +2006,24 @@ func TestFetchChanInfos(t *testing.T) { edgeQuery = append(edgeQuery, chanID.ToUint64()) } + // Add an additional edge that does not exist. The query should skip + // this channel and return only infos for the edges that exist. + edgeQuery = append(edgeQuery, 500) + + // Add an another edge to the query that has been marked as a zombie + // edge. The query should also skip this channel. + zombieChan, zombieChanID := createEdge( + 666, 0, 0, 0, node1, node2, + ) + if err := graph.AddChannelEdge(&zombieChan); err != nil { + t.Fatalf("unable to create channel edge: %v", err) + } + err = graph.DeleteChannelEdge(&zombieChan.ChannelPoint) + if err != nil { + t.Fatalf("unable to delete and mark edge zombie: %v", err) + } + edgeQuery = append(edgeQuery, zombieChanID.ToUint64()) + // We'll now attempt to query for the range of channel ID's we just // inserted into the database. We should get the exact same set of // edges back. From f82b7c9bac5ec690bbbe983f30ef192ebdbe2d53 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Tue, 9 Apr 2019 17:47:12 -0700 Subject: [PATCH 2/2] channeldb/graph: remove MarkEdgeZombie This commit removes the MarkEdgeZombie method from channeldb. This method is currently not used in any live code paths in production, and is only used in unit tests. However, incorrect usage of this method could result in an edge being present in both the zombie and channel indexes, which deviates from any state we would expect to see in production. Removing the method will help mitigate the potential for writing incorrect unit tests in the future, by forcing zombie edges to be created via the relevant, production APIs, e.g. DeleteChannelEdge. The existing unit tests that use this method have been modified to use the DeleteChannelEdge instead. No regressions were discovered in the process. --- channeldb/graph.go | 30 ------------------------------ channeldb/graph_test.go | 29 ++++++++++++++++++----------- 2 files changed, 18 insertions(+), 41 deletions(-) diff --git a/channeldb/graph.go b/channeldb/graph.go index fef94919f..0485cbbff 100644 --- a/channeldb/graph.go +++ b/channeldb/graph.go @@ -3065,36 +3065,6 @@ func (c *ChannelGraph) NewChannelEdgePolicy() *ChannelEdgePolicy { return &ChannelEdgePolicy{db: c.db} } -// MarkEdgeZombie marks an edge as a zombie within the graph's zombie index. -// The public keys should represent the node public keys of the two parties -// involved in the edge. -func (c *ChannelGraph) MarkEdgeZombie(chanID uint64, pubKey1, - pubKey2 [33]byte) error { - - c.cacheMu.Lock() - defer c.cacheMu.Unlock() - - err := c.db.Update(func(tx *bbolt.Tx) error { - edges := tx.Bucket(edgeBucket) - if edges == nil { - return ErrGraphNoEdgesFound - } - zombieIndex, err := edges.CreateBucketIfNotExists(zombieBucket) - if err != nil { - return err - } - return markEdgeZombie(zombieIndex, chanID, pubKey1, pubKey2) - }) - if err != nil { - return err - } - - c.rejectCache.remove(chanID) - c.chanCache.remove(chanID) - - return nil -} - // markEdgeZombie marks an edge as a zombie within our zombie index. The public // keys should represent the node public keys of the two parties involved in the // edge. diff --git a/channeldb/graph_test.go b/channeldb/graph_test.go index 12ff6a1b8..e8c9440e5 100644 --- a/channeldb/graph_test.go +++ b/channeldb/graph_test.go @@ -1750,9 +1750,8 @@ func TestFilterKnownChanIDs(t *testing.T) { if err := graph.AddChannelEdge(&channel); err != nil { t.Fatalf("unable to create channel edge: %v", err) } - if err := graph.MarkEdgeZombie( - chanID.ToUint64(), node1.PubKeyBytes, node2.PubKeyBytes, - ); err != nil { + err := graph.DeleteChannelEdge(&channel.ChannelPoint) + if err != nil { t.Fatalf("unable to mark edge zombie: %v", err) } @@ -2864,20 +2863,28 @@ func TestGraphZombieIndex(t *testing.T) { if err != nil { t.Fatalf("unable to create test vertex: %v", err) } - edge, _, _ := createChannelEdge(db, node1, node2) - // If the graph is not aware of the edge, then it should not be a - // zombie. + // Swap the nodes if the second's pubkey is smaller than the first. + // Without this, the comparisons at the end will fail probabilistically. + if bytes.Compare(node2.PubKeyBytes[:], node1.PubKeyBytes[:]) < 0 { + node1, node2 = node2, node1 + } + + edge, _, _ := createChannelEdge(db, node1, node2) + if err := graph.AddChannelEdge(edge); err != nil { + t.Fatalf("unable to create channel edge: %v", err) + } + + // Since the edge is known the graph and it isn't a zombie, IsZombieEdge + // should not report the channel as a zombie. isZombie, _, _ := graph.IsZombieEdge(edge.ChannelID) if isZombie { t.Fatal("expected edge to not be marked as zombie") } - // If we mark the edge as a zombie, then we should expect to see it - // within the index. - err = graph.MarkEdgeZombie( - edge.ChannelID, node1.PubKeyBytes, node2.PubKeyBytes, - ) + // If we delete the edge and mark it as a zombie, then we should expect + // to see it within the index. + err = graph.DeleteChannelEdge(&edge.ChannelPoint) if err != nil { t.Fatalf("unable to mark edge as zombie: %v", err) }