graph/db: move FilterKnownChanIDs zombie logic up one layer

Here, we move the business logic in FilterKnownChanIDs from the CRUD
layer to the ChannelGraph layer. We also add a test for the logic.

graph/db/graph.go

@@ -1,6 +1,7 @@
 package graphdb
 
 import (
+	"errors"
 	"sync"
 	"time"
 
@@ -386,3 +387,55 @@ func (c *ChannelGraph) PruneGraphNodes() error {
 
 	return nil
 }
+
+// FilterKnownChanIDs takes a set of channel IDs and return the subset of chan
+// ID's that we don't know and are not known zombies of the passed set. In other
+// words, we perform a set difference of our set of chan ID's and the ones
+// passed in. This method can be used by callers to determine the set of
+// channels another peer knows of that we don't.
+func (c *ChannelGraph) FilterKnownChanIDs(chansInfo []ChannelUpdateInfo,
+	isZombieChan func(time.Time, time.Time) bool) ([]uint64, error) {
+
+	unknown, knownZombies, err := c.KVStore.FilterKnownChanIDs(chansInfo)
+	if err != nil {
+		return nil, err
+	}
+
+	for _, info := range knownZombies {
+		// TODO(ziggie): Make sure that for the strict pruning case we
+		// compare the pubkeys and whether the right timestamp is not
+		// older than the `ChannelPruneExpiry`.
+		//
+		// NOTE: The timestamp data has no verification attached to it
+		// in the `ReplyChannelRange` msg so we are trusting this data
+		// at this point. However it is not critical because we are just
+		// removing the channel from the db when the timestamps are more
+		// recent. During the querying of the gossip msg verification
+		// happens as usual. However we should start punishing peers
+		// when they don't provide us honest data ?
+		isStillZombie := isZombieChan(
+			info.Node1UpdateTimestamp, info.Node2UpdateTimestamp,
+		)
+
+		if isStillZombie {
+			continue
+		}
+
+		// If we have marked it as a zombie but the latest update
+		// timestamps could bring it back from the dead, then we mark it
+		// alive, and we let it be added to the set of IDs to query our
+		// peer for.
+		err := c.KVStore.MarkEdgeLive(
+			info.ShortChannelID.ToUint64(),
+		)
+		// Since there is a chance that the edge could have been marked
+		// as "live" between the FilterKnownChanIDs call and the
+		// MarkEdgeLive call, we ignore the error if the edge is already
+		// marked as live.
+		if err != nil && !errors.Is(err, ErrZombieEdgeNotFound) {
+			return nil, err
+		}
+	}
+
+	return unknown, nil
+}

graph/db/graph_test.go

@@ -1919,6 +1919,76 @@ func TestNodeUpdatesInHorizon(t *testing.T) {
 	}
 }
 
+// TestFilterKnownChanIDsZombieRevival tests that if a ChannelUpdateInfo is
+// passed to FilterKnownChanIDs that contains a channel that we have marked as
+// a zombie, then we will mark it as live again if the new ChannelUpdate has
+// timestamps that would make the channel be considered live again.
+//
+// NOTE: this tests focuses on zombie revival. The main logic of
+// FilterKnownChanIDs is tested in TestFilterKnownChanIDs.
+func TestFilterKnownChanIDsZombieRevival(t *testing.T) {
+	t.Parallel()
+
+	graph, err := MakeTestGraph(t)
+	require.NoError(t, err)
+
+	var (
+		scid1 = lnwire.ShortChannelID{BlockHeight: 1}
+		scid2 = lnwire.ShortChannelID{BlockHeight: 2}
+		scid3 = lnwire.ShortChannelID{BlockHeight: 3}
+	)
+
+	isZombie := func(scid lnwire.ShortChannelID) bool {
+		zombie, _, _ := graph.IsZombieEdge(scid.ToUint64())
+		return zombie
+	}
+
+	// Mark channel 1 and 2 as zombies.
+	err = graph.MarkEdgeZombie(scid1.ToUint64(), [33]byte{}, [33]byte{})
+	require.NoError(t, err)
+	err = graph.MarkEdgeZombie(scid2.ToUint64(), [33]byte{}, [33]byte{})
+	require.NoError(t, err)
+
+	require.True(t, isZombie(scid1))
+	require.True(t, isZombie(scid2))
+	require.False(t, isZombie(scid3))
+
+	// Call FilterKnownChanIDs with an isStillZombie call-back that would
+	// result in the current zombies still be considered as zombies.
+	_, err = graph.FilterKnownChanIDs([]ChannelUpdateInfo{
+		{ShortChannelID: scid1},
+		{ShortChannelID: scid2},
+		{ShortChannelID: scid3},
+	}, func(_ time.Time, _ time.Time) bool {
+		return true
+	})
+	require.NoError(t, err)
+
+	require.True(t, isZombie(scid1))
+	require.True(t, isZombie(scid2))
+	require.False(t, isZombie(scid3))
+
+	// Now call it again but this time with a isStillZombie call-back that
+	// would result in channel with SCID 2 no longer being considered a
+	// zombie.
+	_, err = graph.FilterKnownChanIDs([]ChannelUpdateInfo{
+		{ShortChannelID: scid1},
+		{
+			ShortChannelID:       scid2,
+			Node1UpdateTimestamp: time.Unix(1000, 0),
+		},
+		{ShortChannelID: scid3},
+	}, func(t1 time.Time, _ time.Time) bool {
+		return !t1.Equal(time.Unix(1000, 0))
+	})
+	require.NoError(t, err)
+
+	// Show that SCID 2 has been marked as live.
+	require.True(t, isZombie(scid1))
+	require.False(t, isZombie(scid2))
+	require.False(t, isZombie(scid3))
+}
+
 // TestFilterKnownChanIDs tests that we're able to properly perform the set
 // differences of an incoming set of channel ID's, and those that we already
 // know of on disk.

graph/db/kv_store.go

@@ -2155,16 +2155,20 @@ func (c *KVStore) NodeUpdatesInHorizon(startTime,
 // ID's that we don't know and are not known zombies of the passed set. In other
 // words, we perform a set difference of our set of chan ID's and the ones
 // passed in. This method can be used by callers to determine the set of
-// channels another peer knows of that we don't.
+// channels another peer knows of that we don't. The ChannelUpdateInfos for the
-func (c *KVStore) FilterKnownChanIDs(chansInfo []ChannelUpdateInfo,
+// known zombies is also returned.
-	isZombieChan func(time.Time, time.Time) bool) ([]uint64, error) {
+func (c *KVStore) FilterKnownChanIDs(chansInfo []ChannelUpdateInfo) ([]uint64,
+	[]ChannelUpdateInfo, error) {
 
-	var newChanIDs []uint64
+	var (
+		newChanIDs   []uint64
+		knownZombies []ChannelUpdateInfo
+	)
 
 	c.cacheMu.Lock()
 	defer c.cacheMu.Unlock()
 
-	err := kvdb.Update(c.db, func(tx kvdb.RwTx) error {
+	err := kvdb.View(c.db, func(tx kvdb.RTx) error {
 		edges := tx.ReadBucket(edgeBucket)
 		if edges == nil {
 			return ErrGraphNoEdgesFound
@@ -2197,44 +2201,12 @@ func (c *KVStore) FilterKnownChanIDs(chansInfo []ChannelUpdateInfo,
 					zombieIndex, scid,
 				)
 
-				// TODO(ziggie): Make sure that for the strict
+				if isZombie {
-				// pruning case we compare the pubkeys and
+					knownZombies = append(
-				// whether the right timestamp is not older than
+						knownZombies, info,
-				// the `ChannelPruneExpiry`.
-				//
-				// NOTE: The timestamp data has no verification
-				// attached to it in the `ReplyChannelRange` msg
-				// so we are trusting this data at this point.
-				// However it is not critical because we are
-				// just removing the channel from the db when
-				// the timestamps are more recent. During the
-				// querying of the gossip msg verification
-				// happens as usual.
-				// However we should start punishing peers when
-				// they don't provide us honest data ?
-				isStillZombie := isZombieChan(
-					info.Node1UpdateTimestamp,
-					info.Node2UpdateTimestamp,
 					)
 
-				switch {
-				// If the edge is a known zombie and if we
-				// would still consider it a zombie given the
-				// latest update timestamps, then we skip this
-				// channel.
-				case isZombie && isStillZombie:
 					continue
-
-				// Otherwise, if we have marked it as a zombie
-				// but the latest update timestamps could bring
-				// it back from the dead, then we mark it alive,
-				// and we let it be added to the set of IDs to
-				// query our peer for.
-				case isZombie && !isStillZombie:
-					err := c.markEdgeLiveUnsafe(tx, scid)
-					if err != nil {
-						return err
-					}
 				}
 			}
 
@@ -2244,6 +2216,7 @@ func (c *KVStore) FilterKnownChanIDs(chansInfo []ChannelUpdateInfo,
 		return nil
 	}, func() {
 		newChanIDs = nil
+		knownZombies = nil
 	})
 	switch {
 	// If we don't know of any edges yet, then we'll return the entire set
@@ -2254,13 +2227,13 @@ func (c *KVStore) FilterKnownChanIDs(chansInfo []ChannelUpdateInfo,
 			ogChanIDs[i] = info.ShortChannelID.ToUint64()
 		}
 
-		return ogChanIDs, nil
+		return ogChanIDs, nil, nil
 
 	case err != nil:
-		return nil, err
+		return nil, nil, err
 	}
 
-	return newChanIDs, nil
+	return newChanIDs, knownZombies, nil
 }
 
 // ChannelUpdateInfo couples the SCID of a channel with the timestamps of the