routing: extract getEdgeUnifiers

We split up the functionality in getRouteUnifiers into checking that all
edges exist via getEdgeUnifiers and then add a backward pass that will
be responsible for determining the sender amount.

We remove the node pub key from the error string, as in route building
this is duplicate info, which can be determined from the input keys,
further it's not available in the backward pass anymore.

We refactor the BuildRoute test to use the require library and add a
test case for a max HTLC violation on the last hop.

routing/router.go

@@ -1393,13 +1393,12 @@ func (r *ChannelRouter) extractChannelUpdate(
 // channels that satisfy all requirements.
 type ErrNoChannel struct {
 	position int
-	fromNode route.Vertex
 }
 
 // Error returns a human-readable string describing the error.
 func (e ErrNoChannel) Error() string {
 	return fmt.Sprintf("no matching outgoing channel available for "+
-		"node %v (%v)", e.position, e.fromNode)
+		"node index %v", e.position)
 }
 
 // BuildRoute returns a fully specified route based on a list of pubkeys. If
@@ -1429,6 +1428,15 @@ func (r *ChannelRouter) BuildRoute(amt *lnwire.MilliSatoshi,
 
 	sourceNode := r.cfg.SelfNode
 
+	// We check that each node in the route has a connection to others that
+	// can forward in principle.
+	unifiers, err := getEdgeUnifiers(
+		r.cfg.SelfNode, hops, outgoingChans, r.cfg.RoutingGraph,
+	)
+	if err != nil {
+		return nil, err
+	}
+
 	// If no amount is specified, we need to build a route for the minimum
 	// amount that this route can carry.
 	useMinAmt := amt == nil
@@ -1446,16 +1454,15 @@ func (r *ChannelRouter) BuildRoute(amt *lnwire.MilliSatoshi,
 		runningAmt = *amt
 	}
 
-	unifiers, senderAmt, err := getRouteUnifiers(
-		sourceNode, hops, useMinAmt, runningAmt, outgoingChans,
-		r.cfg.RoutingGraph, bandwidthHints,
+	senderAmt, err := senderAmtBackwardPass(
+		unifiers, useMinAmt, runningAmt, bandwidthHints,
 	)
 	if err != nil {
 		return nil, err
 	}
 
 	pathEdges, receiverAmt, err := getPathEdges(
-		sourceNode, senderAmt, unifiers, bandwidthHints, hops,
+		senderAmt, unifiers, bandwidthHints,
 	)
 	if err != nil {
 		return nil, err
@@ -1481,12 +1488,10 @@ func (r *ChannelRouter) BuildRoute(amt *lnwire.MilliSatoshi,
 	)
 }
 
-// getRouteUnifiers returns a list of edge unifiers for the given route.
-func getRouteUnifiers(source route.Vertex, hops []route.Vertex,
-	useMinAmt bool, runningAmt lnwire.MilliSatoshi,
-	outgoingChans map[uint64]struct{}, graph Graph,
-	bandwidthHints *bandwidthManager) ([]*edgeUnifier, lnwire.MilliSatoshi,
-	error) {
+// getEdgeUnifiers returns a list of edge unifiers for the given route.
+func getEdgeUnifiers(source route.Vertex, hops []route.Vertex,
+	outgoingChans map[uint64]struct{},
+	graph Graph) ([]*edgeUnifier, error) {
 
 	// Allocate a list that will contain the edge unifiers for this route.
 	unifiers := make([]*edgeUnifier, len(hops))
@@ -1502,8 +1507,6 @@ func getRouteUnifiers(source route.Vertex, hops []route.Vertex,
 			fromNode = hops[i-1]
 		}
 
-		localChan := i == 0
-
 		// Build unified policies for this hop based on the channels
 		// known in the graph. Don't use inbound fees.
 		//
@@ -1514,19 +1517,34 @@ func getRouteUnifiers(source route.Vertex, hops []route.Vertex,
 
 		err := u.addGraphPolicies(graph)
 		if err != nil {
-			return nil, 0, err
+			return nil, err
 		}
 
 		// Exit if there are no channels.
 		edgeUnifier, ok := u.edgeUnifiers[fromNode]
 		if !ok {
 			log.Errorf("Cannot find policy for node %v", fromNode)
-			return nil, 0, ErrNoChannel{
-				fromNode: fromNode,
-				position: i,
-			}
+			return nil, ErrNoChannel{position: i}
 		}
 
+		unifiers[i] = edgeUnifier
+	}
+
+	return unifiers, nil
+}
+
+// senderAmtBackwardPass determines the amount that should be sent to fulfill
+// min HTLC requirements. The minimal sender amount can be searched for by
+// activating useMinAmt.
+func senderAmtBackwardPass(unifiers []*edgeUnifier, useMinAmt bool,
+	runningAmt lnwire.MilliSatoshi,
+	bandwidthHints *bandwidthManager) (lnwire.MilliSatoshi, error) {
+
+	// Traverse hops backwards to accumulate fees in the running amounts.
+	for i := len(unifiers) - 1; i >= 0; i-- {
+		localChan := i == 0
+		edgeUnifier := unifiers[i]
+
 		// If using min amt, increase amt if needed.
 		if useMinAmt {
 			min := edgeUnifier.minAmt()
@@ -1538,11 +1556,10 @@ func getRouteUnifiers(source route.Vertex, hops []route.Vertex,
 		// Get an edge for the specific amount that we want to forward.
 		edge := edgeUnifier.getEdge(runningAmt, bandwidthHints, 0)
 		if edge == nil {
-			log.Errorf("Cannot find policy with amt=%v for node %v",
-				runningAmt, fromNode)
+			log.Errorf("Cannot find policy with amt=%v for hop %v",
+				runningAmt, i)
 
-			return nil, 0, ErrNoChannel{
-				fromNode: fromNode,
+			return 0, ErrNoChannel{
 				position: i,
 			}
 		}
@@ -1554,19 +1571,16 @@ func getRouteUnifiers(source route.Vertex, hops []route.Vertex,
 
 		log.Tracef("Select channel %v at position %v",
 			edge.policy.ChannelID, i)
-
-		unifiers[i] = edgeUnifier
 	}
 
-	return unifiers, runningAmt, nil
+	return runningAmt, nil
 }
 
 // getPathEdges returns the edges that make up the path and the total amount,
 // including fees, to send the payment.
-func getPathEdges(source route.Vertex, receiverAmt lnwire.MilliSatoshi,
-	unifiers []*edgeUnifier, bandwidthHints *bandwidthManager,
-	hops []route.Vertex) ([]*unifiedEdge,
-	lnwire.MilliSatoshi, error) {
+func getPathEdges(receiverAmt lnwire.MilliSatoshi, unifiers []*edgeUnifier,
+	bandwidthHints *bandwidthManager) ([]*unifiedEdge, lnwire.MilliSatoshi,
+	error) {
 
 	// Now that we arrived at the start of the route and found out the route
 	// total amount, we make a forward pass. Because the amount may have
@@ -1576,15 +1590,7 @@ func getPathEdges(source route.Vertex, receiverAmt lnwire.MilliSatoshi,
 	for i, unifier := range unifiers {
 		edge := unifier.getEdge(receiverAmt, bandwidthHints, 0)
 		if edge == nil {
-			fromNode := source
-			if i > 0 {
-				fromNode = hops[i-1]
-			}
-
-			return nil, 0, ErrNoChannel{
-				fromNode: fromNode,
-				position: i,
-			}
+			return nil, 0, ErrNoChannel{position: i}
 		}
 
 		if i > 0 {

routing/router_test.go

@@ -1536,10 +1536,10 @@ func TestBuildRoute(t *testing.T) {
 		}, 6),
 
 		// Create two channels from b to c. For building routes, we
-		// expect the lowest cost channel to be selected. Note that this
-		// isn't a situation that we are expecting in reality. Routing
-		// nodes are recommended to keep their channel policies towards
-		// the same peer identical.
+		// expect the highest cost channel to be selected. Note that
+		// this isn't a situation that we are expecting in reality.
+		// Routing nodes are recommended to keep their channel policies
+		// towards the same peer identical.
 		symmetricTestChannel("b", "c", chanCapSat, &testChannelPolicy{
 			Expiry:   144,
 			FeeRate:  50000,
@@ -1555,6 +1555,8 @@ func TestBuildRoute(t *testing.T) {
 			Features: paymentAddrFeatures,
 		}, 7),
 
+		// Create some channels that have conflicting min/max
+		// constraints.
 		symmetricTestChannel("a", "e", chanCapSat, &testChannelPolicy{
 			Expiry:   144,
 			FeeRate:  80000,
@@ -1569,9 +1571,28 @@ func TestBuildRoute(t *testing.T) {
 			MaxHTLC:  lnwire.NewMSatFromSatoshis(chanCapSat),
 			Features: paymentAddrFeatures,
 		}, 4),
+
+		// Create some channels that have a conflicting max HTLC
+		// constraint for one node pair, similar to the b->c channels.
+		symmetricTestChannel("b", "z", chanCapSat, &testChannelPolicy{
+			Expiry:   144,
+			FeeRate:  50000,
+			MinHTLC:  lnwire.NewMSatFromSatoshis(20),
+			MaxHTLC:  lnwire.NewMSatFromSatoshis(25),
+			Features: paymentAddrFeatures,
+		}, 3),
+		symmetricTestChannel("b", "z", chanCapSat, &testChannelPolicy{
+			Expiry:   144,
+			FeeRate:  60000,
+			MinHTLC:  lnwire.NewMSatFromSatoshis(20),
+			MaxHTLC:  lnwire.MilliSatoshi(20100),
+			Features: paymentAddrFeatures,
+		}, 8),
 	}
 
-	testGraph, err := createTestGraphFromChannels(t, true, testChannels, "a")
+	testGraph, err := createTestGraphFromChannels(
+		t, true, testChannels, "a",
+	)
 	require.NoError(t, err, "unable to create graph")
 
 	const startingBlockHeight = 101
@@ -1583,15 +1604,10 @@ func TestBuildRoute(t *testing.T) {
 
 		t.Helper()
 
-		if len(rt.Hops) != len(expected) {
-			t.Fatal("hop count mismatch")
-		}
+		require.Len(t, rt.Hops, len(expected))
+
 		for i, hop := range rt.Hops {
-			if hop.ChannelID != expected[i] {
-				t.Fatalf("expected channel %v at pos %v, but "+
-					"got channel %v",
-					expected[i], i, hop.ChannelID)
-			}
+			require.Equal(t, expected[i], hop.ChannelID)
 		}
 
 		lastHop := rt.Hops[len(rt.Hops)-1]
@@ -1603,64 +1619,67 @@ func TestBuildRoute(t *testing.T) {
 	_, err = rand.Read(payAddr[:])
 	require.NoError(t, err)
 
+	// Create hop list for an unknown destination.
+	hops := []route.Vertex{ctx.aliases["b"], ctx.aliases["y"]}
+	_, err = ctx.router.BuildRoute(nil, hops, nil, 40, &payAddr)
+	noChanErr := ErrNoChannel{}
+	require.ErrorAs(t, err, &noChanErr)
+	require.Equal(t, 1, noChanErr.position)
+
 	// Create hop list from the route node pubkeys.
-	hops := []route.Vertex{
-		ctx.aliases["b"], ctx.aliases["c"],
-	}
+	hops = []route.Vertex{ctx.aliases["b"], ctx.aliases["c"]}
 	amt := lnwire.NewMSatFromSatoshis(100)
 
 	// Build the route for the given amount.
-	rt, err := ctx.router.BuildRoute(
-		&amt, hops, nil, 40, &payAddr,
-	)
-	if err != nil {
-		t.Fatal(err)
-	}
+	rt, err := ctx.router.BuildRoute(&amt, hops, nil, 40, &payAddr)
+	require.NoError(t, err)
 
 	// Check that we get the expected route back. The total amount should be
 	// the amount to deliver to hop c (100 sats) plus the max fee for the
 	// connection b->c (6 sats).
 	checkHops(rt, []uint64{1, 7}, payAddr)
-	if rt.TotalAmount != 106000 {
-		t.Fatalf("unexpected total amount %v", rt.TotalAmount)
-	}
+	require.Equal(t, lnwire.MilliSatoshi(106000), rt.TotalAmount)
 
 	// Build the route for the minimum amount.
-	rt, err = ctx.router.BuildRoute(
-		nil, hops, nil, 40, &payAddr,
-	)
-	if err != nil {
-		t.Fatal(err)
-	}
+	rt, err = ctx.router.BuildRoute(nil, hops, nil, 40, &payAddr)
+	require.NoError(t, err)
 
 	// Check that we get the expected route back. The minimum that we can
 	// send from b to c is 20 sats. Hop b charges 1200 msat for the
 	// forwarding. The channel between hop a and b can carry amounts in the
 	// range [5, 100], so 21200 msats is the minimum amount for this route.
 	checkHops(rt, []uint64{1, 7}, payAddr)
-	if rt.TotalAmount != 21200 {
-		t.Fatalf("unexpected total amount %v", rt.TotalAmount)
-	}
+	require.Equal(t, lnwire.MilliSatoshi(21200), rt.TotalAmount)
+
+	// The receiver gets sent the minimal HTLC amount.
+	require.Equal(t, lnwire.MilliSatoshi(20000), rt.Hops[1].AmtToForward)
 
 	// Test a route that contains incompatible channel htlc constraints.
 	// There is no amount that can pass through both channel 5 and 4.
-	hops = []route.Vertex{
-		ctx.aliases["e"], ctx.aliases["c"],
-	}
-	_, err = ctx.router.BuildRoute(
-		nil, hops, nil, 40, nil,
-	)
-	errNoChannel, ok := err.(ErrNoChannel)
-	if !ok {
-		t.Fatalf("expected incompatible policies error, but got %v",
-			err)
-	}
-	if errNoChannel.position != 0 {
-		t.Fatalf("unexpected no channel error position")
-	}
-	if errNoChannel.fromNode != ctx.aliases["a"] {
-		t.Fatalf("unexpected no channel error node")
-	}
+	hops = []route.Vertex{ctx.aliases["e"], ctx.aliases["c"]}
+	_, err = ctx.router.BuildRoute(nil, hops, nil, 40, nil)
+	require.Error(t, err)
+	noChanErr = ErrNoChannel{}
+	require.ErrorAs(t, err, &noChanErr)
+	require.Equal(t, 0, noChanErr.position)
+
+	// Test a route that contains channel constraints that lead to a
+	// different selection of a unified edge, when the amount is rescaled
+	// for the final edge. From a backward pass we expect the policy of
+	// channel 8 to be used, because its policy has the highest fee rate,
+	// bumping the amount to 20000 msat leading to a sender amount of 21200
+	// msat including the fees for hop over channel 8. In the forward pass
+	// however, we discover that the max HTLC constraint of channel 8 is
+	// violated after subtracting the fee, which is why we change the policy
+	// to the one of channel 3. This leads to a delivered amount of 20191
+	// msat, in contrast to the naive 20000 msat from the min HTLC
+	// constraint of both channels.
+	hops = []route.Vertex{ctx.aliases["b"], ctx.aliases["z"]}
+	rt, err = ctx.router.BuildRoute(nil, hops, nil, 40, &payAddr)
+	require.NoError(t, err)
+	checkHops(rt, []uint64{1, 3}, payAddr)
+	require.Equal(t, lnwire.MilliSatoshi(21200), rt.TotalAmount)
+	require.Equal(t, lnwire.MilliSatoshi(20191), rt.Hops[1].AmtToForward)
 }
 
 // TestGetPathEdges tests that the getPathEdges function returns the expected
@@ -1689,14 +1708,13 @@ func TestGetPathEdges(t *testing.T) {
 				localChan: true,
 			},
 		},
-		expectedErr: fmt.Sprintf("no matching outgoing channel "+
-			"available for node 0 (%v)", ctx.aliases["roasbeef"]),
+		expectedErr: fmt.Sprintf("no matching outgoing channel " +
+			"available for node index 0"),
 	}}
 
 	for _, tc := range testCases {
 		pathEdges, amt, err := getPathEdges(
-			tc.sourceNode, tc.amt, tc.unifiers, tc.bandwidthHints,
-			tc.hops,
+			tc.amt, tc.unifiers, tc.bandwidthHints,
 		)
 
 		if tc.expectedErr != "" {