diff --git a/routing/router.go b/routing/router.go index 3d2c34eac..1fce71fb3 100644 --- a/routing/router.go +++ b/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 } diff --git a/routing/router_test.go b/routing/router_test.go index 6b80bd86e..3371678c1 100644 --- a/routing/router_test.go +++ b/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()