routing: backward pass determines unified edges

We shift the duty of determining the policies to the backward pass as
the forward pass will only be responsible for finding the corrected
receiver amount.

Note that this is not a pure refactor as demonstrated in the test, as
the forward pass doesn't select new policies anymore, which is less
flexible and doesn't lead to the highest possible receiver amount. This
is however neccessary as we otherwise won't be able to compute
forwarding amounts involving inbound fees and this edge case is unlikely
to occur, because we search for a min amount for a route that was most
likely constructed for a larger amount.

routing/router.go

@@ -1451,35 +1451,29 @@ func (r *ChannelRouter) BuildRoute(amt *lnwire.MilliSatoshi,
 		// the destination. There are nodes in the wild that have a
 		// min_htlc channel policy of zero, which could lead to a zero
 		// amount payment being made.
-		senderAmt, err := senderAmtBackwardPass(
+		var senderAmt lnwire.MilliSatoshi
+		pathEdges, senderAmt, err = senderAmtBackwardPass(
 			unifiers, useMinAmt, 1, bandwidthHints,
 		)
 		if err != nil {
 			return nil, err
 		}
 
-		pathEdges, receiverAmt, err = getPathEdges(
-			senderAmt, unifiers, bandwidthHints,
-		)
+		receiverAmt, err = receiverAmtForwardPass(senderAmt, pathEdges)
 		if err != nil {
 			return nil, err
 		}
 	} else {
 		// If an amount is specified, we need to build a route that
 		// delivers exactly this amount to the final destination.
-		senderAmt, err := senderAmtBackwardPass(
+		pathEdges, _, err = senderAmtBackwardPass(
 			unifiers, useMinAmt, *amt, bandwidthHints,
 		)
 		if err != nil {
 			return nil, err
 		}
 
-		pathEdges, receiverAmt, err = getPathEdges(
-			senderAmt, unifiers, bandwidthHints,
-		)
-		if err != nil {
-			return nil, err
-		}
+		receiverAmt = *amt
 	}
 
 	// Fetch the current block height outside the routing transaction, to
@@ -1547,12 +1541,15 @@ func getEdgeUnifiers(source route.Vertex, hops []route.Vertex,
 	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.
+// senderAmtBackwardPass returns a list of unified edges for the given route and
+// 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) {
+	bandwidthHints bandwidthHints) ([]*unifiedEdge, lnwire.MilliSatoshi,
+	error) {
+
+	var unifiedEdges = make([]*unifiedEdge, len(unifiers))
 
 	// Traverse hops backwards to accumulate fees in the running amounts.
 	for i := len(unifiers) - 1; i >= 0; i-- {
@@ -1573,7 +1570,7 @@ func senderAmtBackwardPass(unifiers []*edgeUnifier, useMinAmt bool,
 			log.Errorf("Cannot find policy with amt=%v for hop %v",
 				runningAmt, i)
 
-			return 0, ErrNoChannel{
+			return nil, 0, ErrNoChannel{
 				position: i,
 			}
 		}
@@ -1585,37 +1582,37 @@ func senderAmtBackwardPass(unifiers []*edgeUnifier, useMinAmt bool,
 
 		log.Tracef("Select channel %v at position %v",
 			edge.policy.ChannelID, i)
+
+		unifiedEdges[i] = edge
 	}
 
-	return runningAmt, nil
+	return unifiedEdges, runningAmt, nil
 }
 
-// getPathEdges returns the edges that make up the path and the total amount,
-// including fees, to send the payment.
-func getPathEdges(receiverAmt lnwire.MilliSatoshi, unifiers []*edgeUnifier,
-	bandwidthHints *bandwidthManager) ([]*unifiedEdge, lnwire.MilliSatoshi,
-	error) {
+// receiverAmtForwardPass returns the amount that a receiver will receive after
+// deducting all fees from the sender amount.
+func receiverAmtForwardPass(runningAmt lnwire.MilliSatoshi,
+	unifiedEdges []*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
 	// been increased in the backward pass, fees need to be recalculated and
 	// amount ranges re-checked.
-	var pathEdges []*unifiedEdge
-	for i, unifier := range unifiers {
-		edge := unifier.getEdge(receiverAmt, bandwidthHints, 0)
-		if edge == nil {
-			return nil, 0, ErrNoChannel{position: i}
-		}
-
+	for i, edge := range unifiedEdges {
 		if i > 0 {
 			// Decrease the amount to send while going forward.
-			receiverAmt -= edge.policy.ComputeFeeFromIncoming(
-				receiverAmt,
+			runningAmt -= edge.policy.ComputeFeeFromIncoming(
+				runningAmt,
 			)
 		}
 
-		pathEdges = append(pathEdges, edge)
+		if !edge.amtInRange(runningAmt) {
+			log.Errorf("Amount %v not in range for hop %v",
+				runningAmt, i)
+
+			return 0, ErrNoChannel{position: i}
+		}
 	}
 
-	return pathEdges, receiverAmt, nil
+	return runningAmt, nil
 }

routing/router_test.go

@@ -1669,53 +1669,122 @@ func TestBuildRoute(t *testing.T) {
 	// 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.
+	// however, we subtract that fee again, resulting in the min HTLC
+	// amount. The forward pass doesn't check for a different policy that
+	// could me more applicable, which is why we don't get back the highest
+	// amount that could be delivered to the receiver of 21819 msat, using
+	// policy of channel 3.
 	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)
+	checkHops(rt, []uint64{1, 8}, payAddr)
 	require.Equal(t, lnwire.MilliSatoshi(21200), rt.TotalAmount)
-	require.Equal(t, lnwire.MilliSatoshi(20191), rt.Hops[1].AmtToForward)
+	require.Equal(t, lnwire.MilliSatoshi(20000), rt.Hops[1].AmtToForward)
+
 }
 
-// TestGetPathEdges tests that the getPathEdges function returns the expected
-// edges and amount when given a set of unifiers and does not panic.
-func TestGetPathEdges(t *testing.T) {
+// TestReceiverAmtForwardPass tests that the forward pass returns the expected
+// receiver amount when given a set of edges and does not panic.
+func TestReceiverAmtForwardPass(t *testing.T) {
 	t.Parallel()
 
-	const startingBlockHeight = 101
-	ctx := createTestCtxFromFile(t, startingBlockHeight, basicGraphFilePath)
-
 	testCases := []struct {
-		sourceNode     route.Vertex
-		amt            lnwire.MilliSatoshi
-		unifiers       []*edgeUnifier
-		bandwidthHints *bandwidthManager
-		hops           []route.Vertex
+		name         string
+		amt          lnwire.MilliSatoshi
+		unifiedEdges []*unifiedEdge
+		hops         []route.Vertex
 
-		expectedEdges []*models.CachedEdgePolicy
-		expectedAmt   lnwire.MilliSatoshi
-		expectedErr   string
-	}{{
-		sourceNode: ctx.aliases["roasbeef"],
-		unifiers: []*edgeUnifier{
-			{
-				edges:     []*unifiedEdge{},
-				localChan: true,
-			},
+		expectedAmt lnwire.MilliSatoshi
+		expectedErr string
+	}{
+		{
+			name: "empty",
 		},
-		expectedErr: fmt.Sprintf("no matching outgoing channel " +
-			"available for node index 0"),
-	}}
+		{
+			name: "single edge, no valid policy",
+			amt:  1000,
+			unifiedEdges: []*unifiedEdge{
+				{
+					policy: &models.CachedEdgePolicy{
+						MinHTLC: 1001,
+					},
+				},
+			},
+			expectedErr: fmt.Sprintf("no matching outgoing " +
+				"channel available for node index 0"),
+		},
+		{
+			name: "single edge",
+			amt:  1000,
+			unifiedEdges: []*unifiedEdge{
+				{
+					policy: &models.CachedEdgePolicy{
+						MinHTLC: 1000,
+					},
+				},
+			},
+			expectedAmt: 1000,
+		},
+		{
+			name: "outbound fee, no rounding",
+			amt:  1e9,
+			unifiedEdges: []*unifiedEdge{
+				{
+					// The first hop's outbound fee is
+					// irrelevant in fee calculation.
+					policy: &models.CachedEdgePolicy{
+						FeeBaseMSat:               1234,
+						FeeProportionalMillionths: 1234,
+					},
+				},
+				{
+					// No rounding is done here.
+					policy: &models.CachedEdgePolicy{
+						FeeBaseMSat:               1000,
+						FeeProportionalMillionths: 1000,
+					},
+				},
+			},
+			// From an outgoing amount of 999000000 msat, we get
+			// in = out + base + out * rate = 1000000000.0
+			//
+			// The inverse outgoing amount for this is
+			// out = (in - base) / (1 + rate) =
+			// (1e9 - 1000) / (1 + 1e-3) = 999000000.0000001,
+			// which is rounded down.
+			expectedAmt: 999000000,
+		},
+		{
+			name: "outbound fee, rounding",
+			amt:  1e9,
+			unifiedEdges: []*unifiedEdge{
+				{
+					// The first hop's outbound fee is
+					// irrelevant in fee calculation.
+					policy: &models.CachedEdgePolicy{
+						FeeBaseMSat:               1234,
+						FeeProportionalMillionths: 1234,
+					},
+				},
+				{
+					// This policy is chosen such that we
+					// round down.
+					policy: &models.CachedEdgePolicy{
+						FeeBaseMSat:               1000,
+						FeeProportionalMillionths: 999,
+					},
+				},
+			},
+			// The float amount for this is
+			// out = (in - base) / (1 + rate) =
+			// (1e9 - 1000) / (1 + 999e-6) = 999000998.002995,
+			// which is rounded up.
+			expectedAmt: 999000999,
+		},
+	}
 
 	for _, tc := range testCases {
-		pathEdges, amt, err := getPathEdges(
-			tc.amt, tc.unifiers, tc.bandwidthHints,
-		)
+		amt, err := receiverAmtForwardPass(tc.amt, tc.unifiedEdges)
 
 		if tc.expectedErr != "" {
 			require.Error(t, err)
@@ -1725,11 +1794,92 @@ func TestGetPathEdges(t *testing.T) {
 		}
 
 		require.NoError(t, err)
-		require.Equal(t, pathEdges, tc.expectedEdges)
 		require.Equal(t, amt, tc.expectedAmt)
 	}
 }
 
+// TestSenderAmtBackwardPass tests that the computation of the sender amount is
+// done correctly for route building.
+func TestSenderAmtBackwardPass(t *testing.T) {
+	bandwidthHints := bandwidthManager{
+		getLink: func(chanId lnwire.ShortChannelID) (
+			htlcswitch.ChannelLink, error) {
+
+			return nil, nil
+		},
+		localChans: make(map[lnwire.ShortChannelID]struct{}),
+	}
+
+	var (
+		capacity        btcutil.Amount      = 1_000_000
+		testReceiverAmt lnwire.MilliSatoshi = 1_000_000
+		minHTLC         lnwire.MilliSatoshi = 1_000
+	)
+
+	edgeUnifiers := []*edgeUnifier{
+		{
+			edges: []*unifiedEdge{
+				{
+					// This outbound fee doesn't have an
+					// effect (sender doesn't pay outbound).
+					policy: &models.CachedEdgePolicy{
+						FeeBaseMSat: 112,
+					},
+					capacity: capacity,
+				},
+			},
+		},
+		{
+			edges: []*unifiedEdge{
+				{
+					policy: &models.CachedEdgePolicy{
+						FeeBaseMSat: 222,
+					},
+					capacity: capacity,
+				},
+			},
+		},
+		{
+			edges: []*unifiedEdge{
+				{
+					policy: &models.CachedEdgePolicy{
+						FeeBaseMSat: 333,
+						MinHTLC:     minHTLC,
+					},
+					capacity: capacity,
+				},
+			},
+		},
+	}
+
+	// A search for an amount that is below the minimum HTLC amount should
+	// fail.
+	_, _, err := senderAmtBackwardPass(
+		edgeUnifiers, false, minHTLC-1, &bandwidthHints,
+	)
+	require.Error(t, err)
+
+	// Do a min amount search.
+	unifiedEdges, senderAmount, err := senderAmtBackwardPass(
+		edgeUnifiers, true, 1, &bandwidthHints,
+	)
+	require.NoError(t, err)
+	require.Equal(t, minHTLC+333+222, senderAmount)
+
+	// Do a search for a specific amount.
+	unifiedEdges, senderAmount, err = senderAmtBackwardPass(
+		edgeUnifiers, false, testReceiverAmt, &bandwidthHints,
+	)
+	require.NoError(t, err)
+	require.Equal(t, testReceiverAmt+333+222, senderAmount)
+
+	// Check that we arrive at the same receiver amount by doing a forward
+	// pass.
+	receiverAmt, err := receiverAmtForwardPass(senderAmount, unifiedEdges)
+	require.NoError(t, err)
+	require.Equal(t, testReceiverAmt, receiverAmt)
+}
+
 // TestSendToRouteSkipTempErrSuccess validates a successful payment send.
 func TestSendToRouteSkipTempErrSuccess(t *testing.T) {
 	t.Parallel()