From 78bdcbb115a15b4790b66285f75e1f535388db28 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Wed, 17 Apr 2019 13:22:33 -0700 Subject: [PATCH] routing: prune channels only if both edges are present We do this to ensure we don't prune too aggressively, as it's possible that we've only received the channel announcement for a channel, but not its accompanying channel updates. --- routing/pathfind_test.go | 7 +- routing/router.go | 2 +- routing/router_test.go | 144 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 150 insertions(+), 3 deletions(-) diff --git a/routing/pathfind_test.go b/routing/pathfind_test.go index 0c23bade..0781d1d1 100644 --- a/routing/pathfind_test.go +++ b/routing/pathfind_test.go @@ -15,6 +15,7 @@ import ( "os" "strings" "testing" + "time" "github.com/btcsuite/btcd/btcec" "github.com/btcsuite/btcd/chaincfg/chainhash" @@ -303,6 +304,7 @@ type testChannelPolicy struct { MaxHTLC lnwire.MilliSatoshi FeeBaseMsat lnwire.MilliSatoshi FeeRate lnwire.MilliSatoshi + LastUpdate time.Time } type testChannelEnd struct { @@ -319,6 +321,7 @@ func defaultTestChannelEnd(alias string, capacity btcutil.Amount) *testChannelEn MaxHTLC: lnwire.NewMSatFromSatoshis(capacity), FeeBaseMsat: lnwire.MilliSatoshi(1000), FeeRate: lnwire.MilliSatoshi(1), + LastUpdate: testTime, }, } } @@ -500,7 +503,7 @@ func createTestGraphFromChannels(testChannels []*testChannel) (*testGraphInstanc MessageFlags: msgFlags, ChannelFlags: 0, ChannelID: channelID, - LastUpdate: testTime, + LastUpdate: testChannel.Node1.LastUpdate, TimeLockDelta: testChannel.Node1.Expiry, MinHTLC: testChannel.Node1.MinHTLC, MaxHTLC: testChannel.Node1.MaxHTLC, @@ -522,7 +525,7 @@ func createTestGraphFromChannels(testChannels []*testChannel) (*testGraphInstanc MessageFlags: msgFlags, ChannelFlags: lnwire.ChanUpdateDirection, ChannelID: channelID, - LastUpdate: testTime, + LastUpdate: testChannel.Node2.LastUpdate, TimeLockDelta: testChannel.Node2.Expiry, MinHTLC: testChannel.Node2.MinHTLC, MaxHTLC: testChannel.Node2.MaxHTLC, diff --git a/routing/router.go b/routing/router.go index e1019dc9..b805c704 100644 --- a/routing/router.go +++ b/routing/router.go @@ -638,7 +638,7 @@ func (r *ChannelRouter) pruneZombieChans() error { // If *both* edges haven't been updated for a period of // chanExpiry, then we'll mark the channel itself as eligible // for graph pruning. - e1Zombie, e2Zombie := true, true + var e1Zombie, e2Zombie bool if e1 != nil { e1Zombie = time.Since(e1.LastUpdate) >= chanExpiry if e1Zombie { diff --git a/routing/router_test.go b/routing/router_test.go index df9ad9e6..5494d06d 100644 --- a/routing/router_test.go +++ b/routing/router_test.go @@ -1994,6 +1994,109 @@ func TestRouterChansClosedOfflinePruneGraph(t *testing.T) { } } +// TestPruneChannelGraphStaleEdges ensures that we properly prune stale edges +// from the channel graph. +func TestPruneChannelGraphStaleEdges(t *testing.T) { + t.Parallel() + + freshTimestamp := time.Now() + staleTimestamp := time.Time{} + + // We'll create the following test graph so that only the last channel + // is pruned. + testChannels := []*testChannel{ + // No edges. + { + Node1: &testChannelEnd{Alias: "a"}, + Node2: &testChannelEnd{Alias: "b"}, + Capacity: 100000, + ChannelID: 1, + }, + + // Only one edge with a stale timestamp. + { + Node1: &testChannelEnd{ + Alias: "a", + testChannelPolicy: &testChannelPolicy{ + LastUpdate: staleTimestamp, + }, + }, + Node2: &testChannelEnd{Alias: "b"}, + Capacity: 100000, + ChannelID: 2, + }, + + // Only one edge with a fresh timestamp. + { + Node1: &testChannelEnd{ + Alias: "a", + testChannelPolicy: &testChannelPolicy{ + LastUpdate: freshTimestamp, + }, + }, + Node2: &testChannelEnd{Alias: "b"}, + Capacity: 100000, + ChannelID: 3, + }, + + // One edge fresh, one edge stale. + { + Node1: &testChannelEnd{ + Alias: "c", + testChannelPolicy: &testChannelPolicy{ + LastUpdate: freshTimestamp, + }, + }, + Node2: &testChannelEnd{ + Alias: "d", + testChannelPolicy: &testChannelPolicy{ + LastUpdate: staleTimestamp, + }, + }, + Capacity: 100000, + ChannelID: 4, + }, + + // Both edges fresh. + symmetricTestChannel("g", "h", 100000, &testChannelPolicy{ + LastUpdate: freshTimestamp, + }, 5), + + // Both edges stale, only one pruned. + symmetricTestChannel("e", "f", 100000, &testChannelPolicy{ + LastUpdate: staleTimestamp, + }, 6), + } + + // We'll create our test graph and router backed with these test + // channels we've created. + testGraph, err := createTestGraphFromChannels(testChannels) + if err != nil { + t.Fatalf("unable to create test graph: %v", err) + } + defer testGraph.cleanUp() + + const startingHeight = 100 + ctx, cleanUp, err := createTestCtxFromGraphInstance( + startingHeight, testGraph, + ) + if err != nil { + t.Fatalf("unable to create test context: %v", err) + } + defer cleanUp() + + // All of the channels should exist before pruning them. + assertChannelsPruned(t, ctx.graph, testChannels) + + // Proceed to prune the channels - only the last one should be pruned. + if err := ctx.router.pruneZombieChans(); err != nil { + t.Fatalf("unable to prune zombie channels: %v", err) + } + + prunedChannel := testChannels[len(testChannels)-1].ChannelID + assertChannelsPruned(t, ctx.graph, testChannels, prunedChannel) +} + // TestFindPathFeeWeighting tests that the findPath method will properly prefer // routes with lower fees over routes with lower time lock values. This is // meant to exercise the fact that the internal findPath method ranks edges @@ -2297,3 +2400,44 @@ func TestEmptyRoutesGenerateSphinxPacket(t *testing.T) { t.Fatalf("expected empty hops error: instead got: %v", err) } } + +// assertChannelsPruned ensures that only the given channels are pruned from the +// graph out of the set of all channels. +func assertChannelsPruned(t *testing.T, graph *channeldb.ChannelGraph, + channels []*testChannel, prunedChanIDs ...uint64) { + + t.Helper() + + pruned := make(map[uint64]struct{}, len(channels)) + for _, chanID := range prunedChanIDs { + pruned[chanID] = struct{}{} + } + + for _, channel := range channels { + _, shouldPrune := pruned[channel.ChannelID] + _, _, exists, isZombie, err := graph.HasChannelEdge( + channel.ChannelID, + ) + if err != nil { + t.Fatalf("unable to determine existence of "+ + "channel=%v in the graph: %v", + channel.ChannelID, err) + } + if !shouldPrune && !exists { + t.Fatalf("expected channel=%v to exist within "+ + "the graph", channel.ChannelID) + } + if shouldPrune && exists { + t.Fatalf("expected channel=%v to not exist "+ + "within the graph", channel.ChannelID) + } + if !shouldPrune && isZombie { + t.Fatalf("expected channel=%v to not be marked "+ + "as zombie", channel.ChannelID) + } + if shouldPrune && !isZombie { + t.Fatalf("expected channel=%v to be marked as "+ + "zombie", channel.ChannelID) + } + } +}