Merge pull request #1358 from joostjager/newfee

routing: routing may come up with suboptimal routes (weight function)

routing/pathfind.go

@@ -26,6 +26,19 @@ const (
 
 	// infinity is used as a starting distance in our shortest path search.
 	infinity = math.MaxInt64
+
+	// RiskFactorBillionths controls the influence of time lock delta
+	// of a channel on route selection. It is expressed as billionths
+	// of msat per msat sent through the channel per time lock delta
+	// block. See edgeWeight function below for more details.
+	// The chosen value is based on the previous incorrect weight function
+	// 1 + timelock + fee * fee. In this function, the fee penalty
+	// diminishes the time lock penalty for all but the smallest amounts.
+	// To not change the behaviour of path finding too drastically, a
+	// relatively small value is chosen which is still big enough to give
+	// some effect with smaller time lock values. The value may need
+	// tweaking and/or be made configurable in the future.
+	RiskFactorBillionths = 15
 )
 
 // HopHint is a routing hint that contains the minimum information of a channel
@@ -425,29 +438,24 @@ type edgeWithPrev struct {
 }
 
 // edgeWeight computes the weight of an edge. This value is used when searching
-// for the shortest path within the channel graph between two nodes. Currently
-// a component is just 1 + the cltv delta value required at this hop, this
-// value should be tuned with experimental and empirical data. We'll also
-// factor in the "pure fee" through this hop, using the square of this fee as
-// part of the weighting. The goal here is to bias more heavily towards fee
-// ranking, and fallback to a time-lock based value in the case of a fee tie.
-//
-// TODO(roasbeef): compute robust weight metric
+// for the shortest path within the channel graph between two nodes. Weight is
+// is the fee itself plus a time lock penalty added to it. This benefits 
+// channels with shorter time lock deltas and shorter (hops) routes in general. 
+// RiskFactor controls the influence of time lock on route selection. This is 
+// currently a fixed value, but might be configurable in the future.
 func edgeWeight(amt lnwire.MilliSatoshi, e *channeldb.ChannelEdgePolicy) int64 {
 	// First, we'll compute the "pure" fee through this hop. We say pure,
 	// as this may not be what's ultimately paid as fees are properly
 	// calculated backwards, while we're going in the reverse direction.
-	pureFee := computeFee(amt, e)
+	pureFee := int64(computeFee(amt, e))
 
-	// We'll then square the fee itself in order to more heavily weight our
-	// edge selection to bias towards lower fees.
-	feeWeight := int64(pureFee * pureFee)
+	// timeLockPenalty is the penalty for the time lock delta of this channel.
+	// It is controlled by RiskFactorBillionths and scales proportional
+	// to the amount that will pass through channel. Rationale is that it if
+	// a twice as large amount gets locked up, it is twice as bad.
+	timeLockPenalty := int64(amt) * int64(e.TimeLockDelta) * RiskFactorBillionths / 1000000000
 
-	// The final component is then 1 plus the timelock delta.
-	timeWeight := int64(1 + e.TimeLockDelta)
-
-	// The final weighting is: fee^2 + time_lock_delta.
-	return feeWeight + timeWeight
+	return pureFee + timeLockPenalty
 }
 
 // findPath attempts to find a path from the source node within the

routing/pathfind_test.go

@@ -2,6 +2,7 @@ package routing
 
 import (
 	"bytes"
+	"crypto/sha256"
 	"encoding/binary"
 	"encoding/hex"
 	"encoding/json"
@@ -288,6 +289,275 @@ func parseTestGraph(path string) (*channeldb.ChannelGraph, func(), aliasMap, err
 	return graph, cleanUp, aliasMap, nil
 }
 
+type testChannelPolicy struct {
+	Expiry      uint16
+	MinHTLC     lnwire.MilliSatoshi
+	FeeBaseMsat lnwire.MilliSatoshi
+	FeeRate     lnwire.MilliSatoshi
+}
+
+type testChannelEnd struct {
+	Alias string
+	testChannelPolicy
+}
+
+func defaultTestChannelEnd(alias string) *testChannelEnd {
+	return &testChannelEnd{
+		Alias: alias,
+		testChannelPolicy: testChannelPolicy{
+			Expiry:      144,
+			MinHTLC:     lnwire.MilliSatoshi(1000),
+			FeeBaseMsat: lnwire.MilliSatoshi(1000),
+			FeeRate:     lnwire.MilliSatoshi(1),
+		},
+	}
+}
+
+func symmetricTestChannel(alias1 string, alias2 string, capacity btcutil.Amount,
+	policy *testChannelPolicy) *testChannel {
+	return &testChannel{
+		Capacity: capacity,
+		Node1: &testChannelEnd{
+			Alias:             alias1,
+			testChannelPolicy: *policy,
+		},
+		Node2: &testChannelEnd{
+			Alias:             alias2,
+			testChannelPolicy: *policy,
+		},
+	}
+}
+
+type testChannel struct {
+	Node1    *testChannelEnd
+	Node2    *testChannelEnd
+	Capacity btcutil.Amount
+}
+
+// createTestGraph returns a fully populated ChannelGraph based on a set of
+// test channels. Additional required information like keys are derived in
+// a deterministical way and added to the channel graph. A list of nodes is
+// not required and derived from the channel data. The goal is to keep
+// instantiating a test channel graph as light weight as possible.
+func createTestGraph(testChannels []*testChannel) (*channeldb.ChannelGraph, func(), aliasMap, error) {
+	// We'll use this fake address for the IP address of all the nodes in
+	// our tests. This value isn't needed for path finding so it doesn't
+	// need to be unique.
+	var testAddrs []net.Addr
+	testAddr, err := net.ResolveTCPAddr("tcp", "192.0.0.1:8888")
+	if err != nil {
+		return nil, nil, nil, err
+	}
+	testAddrs = append(testAddrs, testAddr)
+
+	// Next, create a temporary graph database for usage within the test.
+	graph, cleanUp, err := makeTestGraph()
+	if err != nil {
+		return nil, nil, nil, err
+	}
+
+	aliasMap := make(map[string]*btcec.PublicKey)
+
+	nodeIndex := byte(0)
+	addNodeWithAlias := func(alias string) (*channeldb.LightningNode, error) {
+		keyBytes := make([]byte, 32)
+		keyBytes = []byte{
+			0, 0, 0, 0, 0, 0, 0, 0,
+			0, 0, 0, 0, 0, 0, 0, 0,
+			0, 0, 0, 0, 0, 0, 0, 0,
+			0, 0, 0, 0, 0, 0, 0, nodeIndex + 1,
+		}
+
+		_, pubKey := btcec.PrivKeyFromBytes(btcec.S256(),
+			keyBytes)
+
+		dbNode := &channeldb.LightningNode{
+			HaveNodeAnnouncement: true,
+			AuthSigBytes:         testSig.Serialize(),
+			LastUpdate:           time.Now(),
+			Addresses:            testAddrs,
+			Alias:                alias,
+			Features:             testFeatures,
+		}
+
+		copy(dbNode.PubKeyBytes[:], pubKey.SerializeCompressed())
+
+		// With the node fully parsed, add it as a vertex within the
+		// graph.
+		if err := graph.AddLightningNode(dbNode); err != nil {
+			return nil, err
+		}
+
+		aliasMap[alias] = pubKey
+		nodeIndex++
+
+		return dbNode, nil
+	}
+
+	var source *channeldb.LightningNode
+	if source, err = addNodeWithAlias("roasbeef"); err != nil {
+		return nil, nil, nil, err
+	}
+
+	// Set the source node
+	if err := graph.SetSourceNode(source); err != nil {
+		return nil, nil, nil, err
+	}
+
+	channelID := uint64(0)
+	for _, testChannel := range testChannels {
+		for _, alias := range []string{
+			testChannel.Node1.Alias, testChannel.Node2.Alias} {
+
+			_, exists := aliasMap[alias]
+			if !exists {
+				addNodeWithAlias(alias)
+			}
+		}
+
+		var hash [sha256.Size]byte
+		hash[len(hash)-1] = byte(channelID)
+
+		fundingPoint := &wire.OutPoint{
+			Hash:  chainhash.Hash(hash),
+			Index: 0,
+		}
+
+		// We first insert the existence of the edge between the two
+		// nodes.
+		edgeInfo := channeldb.ChannelEdgeInfo{
+			ChannelID:    channelID,
+			AuthProof:    &testAuthProof,
+			ChannelPoint: *fundingPoint,
+			Capacity:     testChannel.Capacity,
+		}
+
+		node1Bytes := aliasMap[testChannel.Node1.Alias].SerializeCompressed()
+		node2Bytes := aliasMap[testChannel.Node2.Alias].SerializeCompressed()
+
+		copy(edgeInfo.NodeKey1Bytes[:], node1Bytes)
+		copy(edgeInfo.NodeKey2Bytes[:], node2Bytes)
+		copy(edgeInfo.BitcoinKey1Bytes[:], node1Bytes)
+		copy(edgeInfo.BitcoinKey2Bytes[:], node2Bytes)
+
+		err = graph.AddChannelEdge(&edgeInfo)
+		if err != nil && err != channeldb.ErrEdgeAlreadyExist {
+			return nil, nil, nil, err
+		}
+
+		edgePolicy := &channeldb.ChannelEdgePolicy{
+			SigBytes:                  testSig.Serialize(),
+			Flags:                     lnwire.ChanUpdateFlag(0),
+			ChannelID:                 channelID,
+			LastUpdate:                time.Now(),
+			TimeLockDelta:             testChannel.Node1.Expiry,
+			MinHTLC:                   testChannel.Node1.MinHTLC,
+			FeeBaseMSat:               testChannel.Node1.FeeBaseMsat,
+			FeeProportionalMillionths: testChannel.Node1.FeeRate,
+		}
+		if err := graph.UpdateEdgePolicy(edgePolicy); err != nil {
+			return nil, nil, nil, err
+		}
+
+		edgePolicy = &channeldb.ChannelEdgePolicy{
+			SigBytes:                  testSig.Serialize(),
+			Flags:                     lnwire.ChanUpdateFlag(lnwire.ChanUpdateDirection),
+			ChannelID:                 channelID,
+			LastUpdate:                time.Now(),
+			TimeLockDelta:             testChannel.Node2.Expiry,
+			MinHTLC:                   testChannel.Node2.MinHTLC,
+			FeeBaseMSat:               testChannel.Node2.FeeBaseMsat,
+			FeeProportionalMillionths: testChannel.Node2.FeeRate,
+		}
+
+		if err := graph.UpdateEdgePolicy(edgePolicy); err != nil {
+			return nil, nil, nil, err
+		}
+
+		channelID++
+	}
+
+	return graph, cleanUp, aliasMap, nil
+}
+
+// TestFindLowestFeePath tests that out of two routes with identical total
+// time lock values, the route with the lowest total fee should be returned.
+// The fee rates are chosen such that the test failed on the previous edge
+// weight function where one of the terms was fee squared.
+func TestFindLowestFeePath(t *testing.T) {
+	t.Parallel()
+
+	// Set up a test graph with two paths from roasbeef to target. Both
+	// paths have equal total time locks, but the path through b has lower
+	// fees (700 compared to 800 for the path through a).
+	testChannels := []*testChannel{
+		symmetricTestChannel("roasbeef", "a", 100000, &testChannelPolicy{
+			Expiry:  144,
+			FeeRate: 400,
+			MinHTLC: 1,
+		}),
+		symmetricTestChannel("a", "target", 100000, &testChannelPolicy{
+			Expiry:  144,
+			FeeRate: 400,
+			MinHTLC: 1,
+		}),
+		symmetricTestChannel("roasbeef", "b", 100000, &testChannelPolicy{
+			Expiry:  144,
+			FeeRate: 100,
+			MinHTLC: 1,
+		}),
+		symmetricTestChannel("b", "target", 100000, &testChannelPolicy{
+			Expiry:  144,
+			FeeRate: 600,
+			MinHTLC: 1,
+		}),
+	}
+
+	graph, cleanUp, aliases, err := createTestGraph(testChannels)
+	defer cleanUp()
+	if err != nil {
+		t.Fatalf("unable to create graph: %v", err)
+	}
+
+	sourceNode, err := graph.SourceNode()
+	if err != nil {
+		t.Fatalf("unable to fetch source node: %v", err)
+	}
+	sourceVertex := Vertex(sourceNode.PubKeyBytes)
+
+	ignoredEdges := make(map[uint64]struct{})
+	ignoredVertexes := make(map[Vertex]struct{})
+
+	const (
+		startingHeight = 100
+		finalHopCLTV   = 1
+	)
+
+	paymentAmt := lnwire.NewMSatFromSatoshis(100)
+	target := aliases["target"]
+	path, err := findPath(
+		nil, graph, nil, sourceNode, target, ignoredVertexes,
+		ignoredEdges, paymentAmt, nil,
+	)
+	if err != nil {
+		t.Fatalf("unable to find path: %v", err)
+	}
+	route, err := newRoute(
+		paymentAmt, infinity, sourceVertex, path, startingHeight, 
+		finalHopCLTV)
+	if err != nil {
+		t.Fatalf("unable to create path: %v", err)
+	}
+
+	// Assert that the lowest fee route is returned.	
+	if !bytes.Equal(route.Hops[0].Channel.Node.PubKeyBytes[:],
+		aliases["b"].SerializeCompressed()) {
+		t.Fatalf("expected route to pass through b, "+
+			"but got a route through %v",
+			route.Hops[0].Channel.Node.Alias)
+	}
+}
+
 func TestBasicGraphPathFinding(t *testing.T) {
 	t.Parallel()