From 838975fca9b6cda345a913bc650acac0c30783b7 Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Wed, 14 Sep 2022 16:43:48 +0200 Subject: [PATCH 1/3] routing: do not use newRoute in BuildRoute --- routing/router.go | 94 +++++++++++++++++++++++++++-------------------- 1 file changed, 54 insertions(+), 40 deletions(-) diff --git a/routing/router.go b/routing/router.go index 348914db73..76536c066a 100644 --- a/routing/router.go +++ b/routing/router.go @@ -2729,11 +2729,16 @@ func (r *ChannelRouter) BuildRoute(amt *lnwire.MilliSatoshi, return nil, err } - // Allocate a list that will contain the unified policies for this - // route. - edges := make([]*unifiedPolicy, len(hops)) + var ( + routeHops []*route.Hop + + // timeLock will accumulate the cumulative time lock across the + // entire route. + timeLock = height + + runningAmt lnwire.MilliSatoshi + ) - var runningAmt lnwire.MilliSatoshi if useMinAmt { // For minimum amount routes, aim to deliver at least 1 msat to // the destination. There are nodes in the wild that have a @@ -2758,7 +2763,12 @@ func (r *ChannelRouter) BuildRoute(amt *lnwire.MilliSatoshi, fromNode = hops[i-1] } - localChan := i == 0 + // Initialize a route hop for toNode. + currentHop := &route.Hop{ + PubKeyBytes: toNode, + AmtToForward: runningAmt, + OutgoingTimeLock: uint32(timeLock), + } // Build unified policies for this hop based on the channels // known in the graph. @@ -2796,50 +2806,54 @@ func (r *ChannelRouter) BuildRoute(amt *lnwire.MilliSatoshi, } } - // Add fee for this hop. - if !localChan { - runningAmt += policy.ComputeFee(runningAmt) + log.Tracef("Select channel %v at position %v", policy.ChannelID, i) + + // Define a helper function that checks this edge's feature + // vector for support for a given feature. We assume at this + // point that the feature vectors transitive dependencies have + // been validated. + supports := func(feature lnwire.FeatureBit) bool { + // If this edge comes from router hints, the features + // could be nil. + if policy.ToNodeFeatures == nil { + return false + } + + return policy.ToNodeFeatures.HasFeature(feature) } - log.Tracef("Select channel %v at position %v", policy.ChannelID, i) + // Store the ID for the selected channel. + currentHop.ChannelID = policy.ChannelID - edges[i] = unifiedPolicy - } + // Store the payload type. + tlvPayload := supports(lnwire.TLVOnionPayloadOptional) + currentHop.LegacyPayload = !tlvPayload - // 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 []*channeldb.CachedEdgePolicy - receiverAmt := runningAmt - for i, edge := range edges { - policy := edge.getPolicy(receiverAmt, bandwidthHints) - if policy == nil { - return nil, ErrNoChannel{ - fromNode: hops[i-1], - position: i, + // Store MPP record for final hop if requested and possible. + isFinalHop := i == len(hops)-1 + if isFinalHop && payAddr != nil { + // If we're attaching a payment addr but the receiver + // doesn't support both TLV and payment addrs, fail. + if !supports(lnwire.PaymentAddrOptional) { + return nil, errors.New("cannot attach " + + "payment addr") } - } - if i > 0 { - // Decrease the amount to send while going forward. - receiverAmt -= policy.ComputeFeeFromIncoming( - receiverAmt, - ) + currentHop.MPP = record.NewMPP(runningAmt, *payAddr) } - pathEdges = append(pathEdges, policy) + routeHops = append([]*route.Hop{currentHop}, routeHops...) + + // Add fee for this hop and update time lock. + localChan := i == 0 + if !localChan { + runningAmt += policy.ComputeFee(runningAmt) + timeLock += int32(policy.TimeLockDelta) + } } - // Build and return the final route. - return newRoute( - source, pathEdges, uint32(height), - finalHopParams{ - amt: receiverAmt, - totalAmt: receiverAmt, - cltvDelta: uint16(finalCltvDelta), - records: nil, - paymentAddr: payAddr, - }, + // With the base routing data expressed as hops, build the full route + return route.NewRouteFromHops( + runningAmt, uint32(timeLock), source, routeHops, ) } From c5473127646bcf1c1470bc199122873329d4109c Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Thu, 15 Sep 2022 09:57:32 +0200 Subject: [PATCH 2/3] routing: do not use newRoute in pathfinding --- routing/pathfind.go | 91 +++++++++++++++++++++++++------- routing/pathfind_test.go | 92 ++++++++++++--------------------- routing/payment_session.go | 23 ++------- routing/payment_session_test.go | 15 ++---- routing/router.go | 16 +----- routing/router_test.go | 8 +-- 6 files changed, 118 insertions(+), 127 deletions(-) diff --git a/routing/pathfind.go b/routing/pathfind.go index a92d0a7e52..0d449d0ab9 100644 --- a/routing/pathfind.go +++ b/routing/pathfind.go @@ -42,7 +42,7 @@ const ( type pathFinder = func(g *graphParams, r *RestrictParams, cfg *PathFindingConfig, source, target route.Vertex, amt lnwire.MilliSatoshi, timePref float64, finalHtlcExpiry int32) ( - []*channeldb.CachedEdgePolicy, error) + *route.Route, error) var ( // DefaultAttemptCost is the default fixed virtual cost in path finding @@ -426,7 +426,7 @@ func getOutgoingBalance(node route.Vertex, outgoingChans map[uint64]struct{}, // available bandwidth. func findPath(g *graphParams, r *RestrictParams, cfg *PathFindingConfig, source, target route.Vertex, amt lnwire.MilliSatoshi, timePref float64, - finalHtlcExpiry int32) ([]*channeldb.CachedEdgePolicy, error) { + finalHtlcExpiry int32) (*route.Route, error) { // Pathfinding can be a significant portion of the total payment // latency, especially on low-powered devices. Log several metrics to @@ -567,8 +567,9 @@ func findPath(g *graphParams, r *RestrictParams, cfg *PathFindingConfig, LegacyPayload: !features.HasFeature( lnwire.TLVOnionPayloadOptional, ), - MPP: mpp, - Metadata: r.Metadata, + MPP: mpp, + Metadata: r.Metadata, + PubKeyBytes: target, } // We can't always assume that the end destination is publicly @@ -916,7 +917,7 @@ func findPath(g *graphParams, r *RestrictParams, cfg *PathFindingConfig, // Use the distance map to unravel the forward path from source to // target. - var pathEdges []*channeldb.CachedEdgePolicy + var pathEdges []*nodeWithDist currentNode := source for { // Determine the next hop forward using the next map. @@ -928,7 +929,7 @@ func findPath(g *graphParams, r *RestrictParams, cfg *PathFindingConfig, } // Add the next hop to the list of path edges. - pathEdges = append(pathEdges, currentNodeWithDist.nextHop) + pathEdges = append(pathEdges, currentNodeWithDist) // Advance current node. currentNode = currentNodeWithDist.nextHop.ToNodePubKey() @@ -941,24 +942,74 @@ func findPath(g *graphParams, r *RestrictParams, cfg *PathFindingConfig, } } - // For the final hop, we'll set the node features to those determined - // above. These are either taken from the destination features, e.g. - // virtual or invoice features, or loaded as a fallback from the graph. - // The transitive dependencies were already validated above, so no need - // to do so now. - // - // NOTE: This may overwrite features loaded from the graph if - // destination features were provided. This is fine though, since our - // route construction does not care where the features are actually - // taken from. In the future we may wish to do route construction within - // findPath, and avoid using ChannelEdgePolicy altogether. - pathEdges[len(pathEdges)-1].ToNodeFeatures = features - log.Debugf("Found route: probability=%v, hops=%v, fee=%v", distance[source].probability, len(pathEdges), distance[source].amountToReceive-amt) - return pathEdges, nil + var ( + hops = make([]*route.Hop, len(pathEdges)) + + totalTimeLock int32 + totalAmt lnwire.MilliSatoshi + ) + + for nodeIdx := len(pathEdges) - 1; nodeIdx >= 0; nodeIdx-- { + var ( + currentNodeWithDist = pathEdges[nodeIdx] + edge = currentNodeWithDist.nextHop + nextNode = edge.ToNodePubKey() + ) + + // For the final hop, use the previously constructed hop + // instance. + if nextNode == target { + finalHop.ChannelID = edge.ChannelID + + hops[nodeIdx] = &finalHop + + // Set the amount and timelock for the hop paying to the + // final hop. + totalTimeLock = int32(finalHop.OutgoingTimeLock) + totalAmt = finalHop.AmtToForward + + continue + } + + // Determine tlv payload support. + tlvPayload := edge.ToNodeFeatures != nil && + edge.ToNodeFeatures.HasFeature( + lnwire.TLVOnionPayloadOptional, + ) + + // Build hop structure for the intermediate hop. + intermediateHop := &route.Hop{ + PubKeyBytes: nextNode, + ChannelID: edge.ChannelID, + AmtToForward: totalAmt, + OutgoingTimeLock: uint32(totalTimeLock), + LegacyPayload: !tlvPayload, + } + + hops[nodeIdx] = intermediateHop + + // Update the amount and timelock for the hop paying to this + // hop. + next := pathEdges[nodeIdx+1] + + totalTimeLock = next.incomingCltv + totalAmt = next.amountToReceive + } + + // Construct the full route structure. + route, err := route.NewRouteFromHops( + totalAmt, uint32(totalTimeLock), route.Vertex(source), + hops, + ) + if err != nil { + return nil, err + } + + return route, nil } // getProbabilityBasedDist converts a weight into a distance that takes into diff --git a/routing/pathfind_test.go b/routing/pathfind_test.go index 7f576196a4..ce305d06ca 100644 --- a/routing/pathfind_test.go +++ b/routing/pathfind_test.go @@ -877,9 +877,9 @@ func runFindPathWithMetadata(t *testing.T, useCache bool) { ctx.restrictParams.Metadata = []byte{1, 2, 3} ctx.restrictParams.DestFeatures = tlvFeatures - path, err := ctx.findPath(target, paymentAmt) + route, err := ctx.findPath(target, paymentAmt) require.NoError(t, err) - require.Len(t, path, 1) + require.Len(t, route.Hops, 1) // Assert that no path is found when metadata is too large. ctx.restrictParams.Metadata = make([]byte, 2000) @@ -945,17 +945,8 @@ func runFindLowestFeePath(t *testing.T, useCache bool) { paymentAmt := lnwire.NewMSatFromSatoshis(100) target := ctx.keyFromAlias("target") - path, err := ctx.findPath(target, paymentAmt) + route, err := ctx.findPath(target, paymentAmt) require.NoError(t, err, "unable to find path") - route, err := newRoute( - ctx.source, path, startingHeight, - finalHopParams{ - amt: paymentAmt, - cltvDelta: finalHopCLTV, - records: nil, - }, - ) - require.NoError(t, err, "unable to create path") // Assert that the lowest fee route is returned. if route.Hops[1].PubKeyBytes != ctx.keyFromAlias("b") { @@ -1061,7 +1052,6 @@ func testBasicGraphPathFindingCase(t *testing.T, graphInstance *testGraphInstanc sourceNode, err := graphInstance.graph.SourceNode() require.NoError(t, err, "unable to fetch source node") - sourceVertex := route.Vertex(sourceNode.PubKeyBytes) const ( startingHeight = 100 @@ -1070,7 +1060,7 @@ func testBasicGraphPathFindingCase(t *testing.T, graphInstance *testGraphInstanc paymentAmt := lnwire.NewMSatFromSatoshis(test.paymentAmt) target := graphInstance.aliasMap[test.target] - path, err := dbFindPath( + route, err := dbFindPath( graphInstance.graph, nil, &mockBandwidthHints{}, &RestrictParams{ FeeLimit: test.feeLimit, @@ -1089,16 +1079,6 @@ func testBasicGraphPathFindingCase(t *testing.T, graphInstance *testGraphInstanc } require.NoError(t, err, "unable to find path") - route, err := newRoute( - sourceVertex, path, startingHeight, - finalHopParams{ - amt: paymentAmt, - cltvDelta: finalHopCLTV, - records: nil, - }, - ) - require.NoError(t, err, "unable to create path") - if len(route.Hops) != len(expectedHops) { t.Fatalf("route is of incorrect length, expected %v got %v", expectedHopCount, len(route.Hops)) @@ -1135,6 +1115,8 @@ func testBasicGraphPathFindingCase(t *testing.T, graphInstance *testGraphInstanc t.Fatalf("unable to make hop data: %v", err) } + require.NotNil(t, hopData) + if !bytes.Equal(hopData.NextAddress[:], expectedHop[:]) { t.Fatalf("first hop has incorrect next hop: expected %x, got %x", expectedHop[:], hopData.NextAddress[:]) @@ -1148,6 +1130,7 @@ func testBasicGraphPathFindingCase(t *testing.T, graphInstance *testGraphInstanc hopData, err := sphinxPath[lastHopIndex].HopPayload.HopData() require.NoError(t, err, "unable to create hop data") + require.NotNil(t, hopData) if !bytes.Equal(hopData.NextAddress[:], exitHop[:]) { t.Fatalf("first hop has incorrect next hop: expected %x, got %x", @@ -1244,7 +1227,7 @@ func runPathFindingWithAdditionalEdges(t *testing.T, useCache bool) { } find := func(r *RestrictParams) ( - []*channeldb.CachedEdgePolicy, error) { + *route.Route, error) { return dbFindPath( graph.graph, additionalEdges, &mockBandwidthHints{}, @@ -1713,7 +1696,7 @@ func runDestTLVGraphFallback(t *testing.T, useCache bool) { require.NoError(t, err, "unable to fetch source node") find := func(r *RestrictParams, - target route.Vertex) ([]*channeldb.CachedEdgePolicy, error) { + target route.Vertex) (*route.Route, error) { return dbFindPath( ctx.graph, nil, &mockBandwidthHints{}, @@ -2368,16 +2351,18 @@ func TestPathFindSpecExample(t *testing.T) { } func assertExpectedPath(t *testing.T, aliasMap map[string]route.Vertex, - path []*channeldb.CachedEdgePolicy, nodeAliases ...string) { + route *route.Route, nodeAliases ...string) { + + path := route.Hops if len(path) != len(nodeAliases) { t.Fatal("number of hops and number of aliases do not match") } for i, hop := range path { - if hop.ToNodePubKey() != aliasMap[nodeAliases[i]] { + if hop.PubKeyBytes != aliasMap[nodeAliases[i]] { t.Fatalf("expected %v to be pos #%v in hop, instead "+ - "%v was", nodeAliases[i], i, hop.ToNodePubKey()) + "%v was", nodeAliases[i], i, hop.PubKeyBytes) } } } @@ -2446,9 +2431,11 @@ func runRestrictOutgoingChannel(t *testing.T, useCache bool) { // Find the best path given the restriction to only use channel 2 as the // outgoing channel. ctx.restrictParams.OutgoingChannelIDs = []uint64{outgoingChannelID} - path, err := ctx.findPath(target, paymentAmt) + route, err := ctx.findPath(target, paymentAmt) require.NoError(t, err, "unable to find path") + path := route.Hops + // Assert that the route starts with channel chanSourceB1, in line with // the specified restriction. if path[0].ChannelID != chanSourceB1 { @@ -2462,8 +2449,9 @@ func runRestrictOutgoingChannel(t *testing.T, useCache bool) { ctx.restrictParams.OutgoingChannelIDs = []uint64{ chanSourceB1, chanSourceTarget, } - path, err = ctx.findPath(target, paymentAmt) + route, err = ctx.findPath(target, paymentAmt) require.NoError(t, err, "unable to find path") + path = route.Hops if path[0].ChannelID != chanSourceTarget { t.Fatalf("expected route to pass through channel %v", chanSourceTarget) @@ -2501,8 +2489,9 @@ func runRestrictLastHop(t *testing.T, useCache bool) { // Find the best path given the restriction to use b as the last hop. // This should force pathfinding to not take the lowest cost option. ctx.restrictParams.LastHop = &lastHop - path, err := ctx.findPath(target, paymentAmt) + route, err := ctx.findPath(target, paymentAmt) require.NoError(t, err, "unable to find path") + path := route.Hops if path[0].ChannelID != 3 { t.Fatalf("expected route to pass through channel 3, "+ "but channel %v was selected instead", @@ -2566,7 +2555,7 @@ func testCltvLimit(t *testing.T, useCache bool, limit uint32, target := ctx.keyFromAlias("target") ctx.restrictParams.CltvLimit = limit - path, err := ctx.findPath(target, paymentAmt) + route, err := ctx.findPath(target, paymentAmt) if expectedChannel == 0 { // Finish test if we expect no route. if err == errNoPathFound { @@ -2580,15 +2569,6 @@ func testCltvLimit(t *testing.T, useCache bool, limit uint32, startingHeight = 100 finalHopCLTV = 1 ) - route, err := newRoute( - ctx.source, path, startingHeight, - finalHopParams{ - amt: paymentAmt, - cltvDelta: finalHopCLTV, - records: nil, - }, - ) - require.NoError(t, err, "unable to create path") // Assert that the route starts with the expected channel. if route.Hops[0].ChannelID != expectedChannel { @@ -2771,7 +2751,7 @@ func testProbabilityRouting(t *testing.T, useCache bool, ctx.timePref = timePref - path, err := ctx.findPath( + route, err := ctx.findPath( target, lnwire.NewMSatFromSatoshis(paymentAmt), ) if expectedChan == 0 { @@ -2784,6 +2764,8 @@ func testProbabilityRouting(t *testing.T, useCache bool, t.Fatal(err) } + path := route.Hops + // Assert that the route passes through the expected channel. if path[1].ChannelID != expectedChan { t.Fatalf("expected route to pass through channel %v, "+ @@ -2843,11 +2825,13 @@ func runEqualCostRouteSelection(t *testing.T, useCache bool) { AttemptCost: lnwire.NewMSatFromSatoshis(1), } - path, err := ctx.findPath(target, paymentAmt) + route, err := ctx.findPath(target, paymentAmt) if err != nil { t.Fatal(err) } + path := route.Hops + if path[1].ChannelID != 2 { t.Fatalf("expected route to pass through channel %v, "+ "but channel %v was selected instead", 2, @@ -2899,17 +2883,8 @@ func runNoCycle(t *testing.T, useCache bool) { // Find the best path given the restriction to only use channel 2 as the // outgoing channel. - path, err := ctx.findPath(target, paymentAmt) + route, err := ctx.findPath(target, paymentAmt) require.NoError(t, err, "unable to find path") - route, err := newRoute( - ctx.source, path, startingHeight, - finalHopParams{ - amt: paymentAmt, - cltvDelta: finalHopCLTV, - records: nil, - }, - ) - require.NoError(t, err, "unable to create path") if len(route.Hops) != 2 { t.Fatalf("unexpected route") @@ -3011,8 +2986,7 @@ func (c *pathFindingTestContext) aliasFromKey(pubKey route.Vertex) string { } func (c *pathFindingTestContext) findPath(target route.Vertex, - amt lnwire.MilliSatoshi) ([]*channeldb.CachedEdgePolicy, - error) { + amt lnwire.MilliSatoshi) (*route.Route, error) { return dbFindPath( c.graph, nil, c.bandwidthHints, &c.restrictParams, @@ -3020,9 +2994,11 @@ func (c *pathFindingTestContext) findPath(target route.Vertex, ) } -func (c *pathFindingTestContext) assertPath(path []*channeldb.CachedEdgePolicy, +func (c *pathFindingTestContext) assertPath(route *route.Route, expected []uint64) { + path := route.Hops + if len(path) != len(expected) { c.t.Fatalf("expected path of length %v, but got %v", len(expected), len(path)) @@ -3043,7 +3019,7 @@ func dbFindPath(graph *channeldb.ChannelGraph, bandwidthHints bandwidthHints, r *RestrictParams, cfg *PathFindingConfig, source, target route.Vertex, amt lnwire.MilliSatoshi, timePref float64, - finalHtlcExpiry int32) ([]*channeldb.CachedEdgePolicy, error) { + finalHtlcExpiry int32) (*route.Route, error) { sourceNode, err := graph.SourceNode() if err != nil { diff --git a/routing/payment_session.go b/routing/payment_session.go index f3092ee7b4..0a7d2c9927 100644 --- a/routing/payment_session.go +++ b/routing/payment_session.go @@ -297,7 +297,7 @@ func (p *paymentSession) RequestRoute(maxAmt, feeLimit lnwire.MilliSatoshi, sourceVertex := routingGraph.sourceNode() // Find a route for the current amount. - path, err := p.pathFinder( + route, err := p.pathFinder( &graphParams{ additionalEdges: p.additionalEdges, bandwidthHints: bandwidthHints, @@ -305,7 +305,8 @@ func (p *paymentSession) RequestRoute(maxAmt, feeLimit lnwire.MilliSatoshi, }, restrictions, &p.pathFindingConfig, sourceVertex, p.payment.Target, - maxAmt, p.payment.TimePref, finalHtlcExpiry, + maxAmt, p.payment.TimePref, + finalHtlcExpiry, ) // Close routing graph. @@ -378,24 +379,6 @@ func (p *paymentSession) RequestRoute(maxAmt, feeLimit lnwire.MilliSatoshi, return nil, err } - // With the next candidate path found, we'll attempt to turn - // this into a route by applying the time-lock and fee - // requirements. - route, err := newRoute( - sourceVertex, path, height, - finalHopParams{ - amt: maxAmt, - totalAmt: p.payment.Amount, - cltvDelta: finalCltvDelta, - records: p.payment.DestCustomRecords, - paymentAddr: p.payment.PaymentAddr, - metadata: p.payment.Metadata, - }, - ) - if err != nil { - return nil, err - } - return route, err } } diff --git a/routing/payment_session_test.go b/routing/payment_session_test.go index 4eaa5af660..5b338e3609 100644 --- a/routing/payment_session_test.go +++ b/routing/payment_session_test.go @@ -213,7 +213,7 @@ func TestRequestRoute(t *testing.T) { g *graphParams, r *RestrictParams, cfg *PathFindingConfig, source, target route.Vertex, amt lnwire.MilliSatoshi, timePref float64, - finalHtlcExpiry int32) ([]*channeldb.CachedEdgePolicy, error) { + finalHtlcExpiry int32) (*route.Route, error) { // We expect find path to receive a cltv limit excluding the // final cltv delta (including the block padding). @@ -221,18 +221,11 @@ func TestRequestRoute(t *testing.T) { t.Fatal("wrong cltv limit") } - path := []*channeldb.CachedEdgePolicy{ - { - ToNodePubKey: func() route.Vertex { - return route.Vertex{} - }, - ToNodeFeatures: lnwire.NewFeatureVector( - nil, nil, - ), - }, + route := &route.Route{ + TotalTimeLock: uint32(finalHtlcExpiry), } - return path, nil + return route, nil } route, err := session.RequestRoute( diff --git a/routing/router.go b/routing/router.go index 76536c066a..73500a2c54 100644 --- a/routing/router.go +++ b/routing/router.go @@ -1775,7 +1775,7 @@ func (r *ChannelRouter) FindRoute(source, target route.Vertex, return nil, errors.New("time preference out of range") } - path, err := findPath( + route, err := findPath( &graphParams{ additionalEdges: routeHints, bandwidthHints: bandwidthHints, @@ -1789,20 +1789,6 @@ func (r *ChannelRouter) FindRoute(source, target route.Vertex, return nil, err } - // Create the route with absolute time lock values. - route, err := newRoute( - source, path, uint32(currentHeight), - finalHopParams{ - amt: amt, - totalAmt: amt, - cltvDelta: finalExpiry, - records: destCustomRecords, - }, - ) - if err != nil { - return nil, err - } - go log.Tracef("Obtained path to send %v to %x: %v", amt, target, newLogClosure(func() string { return spew.Sdump(route) diff --git a/routing/router_test.go b/routing/router_test.go index 42683fc9c3..b52fb18601 100644 --- a/routing/router_test.go +++ b/routing/router_test.go @@ -2412,7 +2412,7 @@ func TestFindPathFeeWeighting(t *testing.T) { // We'll now attempt a path finding attempt using this set up. Due to // the edge weighting, we should select the direct path over the 2 hop // path even though the direct path has a higher potential time lock. - path, err := dbFindPath( + route, err := dbFindPath( ctx.graph, nil, &mockBandwidthHints{}, noRestrictions, testPathFindingConfig, @@ -2420,13 +2420,15 @@ func TestFindPathFeeWeighting(t *testing.T) { ) require.NoError(t, err, "unable to find path") + path := route.Hops + // The route that was chosen should be exactly one hop, and should be // directly to luoji. if len(path) != 1 { t.Fatalf("expected path length of 1, instead was: %v", len(path)) } - if path[0].ToNodePubKey() != ctx.aliases["luoji"] { - t.Fatalf("wrong node: %v", path[0].ToNodePubKey()) + if path[0].PubKeyBytes != ctx.aliases["luoji"] { + t.Fatalf("wrong node: %v", path[0].PubKeyBytes) } } From c18f8a2306b8a66ee7bd17d4d99d15cf3d29caa1 Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Thu, 15 Sep 2022 09:59:48 +0200 Subject: [PATCH 3/3] routing: remove newRoute --- routing/pathfind.go | 184 ----------------------- routing/pathfind_test.go | 314 --------------------------------------- 2 files changed, 498 deletions(-) diff --git a/routing/pathfind.go b/routing/pathfind.go index 0d449d0ab9..e1eedc5a33 100644 --- a/routing/pathfind.go +++ b/routing/pathfind.go @@ -2,7 +2,6 @@ package routing import ( "container/heap" - "errors" "fmt" "math" "time" @@ -79,189 +78,6 @@ type edgePolicyWithSource struct { edge *channeldb.CachedEdgePolicy } -// finalHopParams encapsulates various parameters for route construction that -// apply to the final hop in a route. These features include basic payment data -// such as amounts and cltvs, as well as more complex features like destination -// custom records and payment address. -type finalHopParams struct { - amt lnwire.MilliSatoshi - totalAmt lnwire.MilliSatoshi - cltvDelta uint16 - records record.CustomSet - paymentAddr *[32]byte - - // metadata is additional data that is sent along with the payment to - // the payee. - metadata []byte -} - -// newRoute constructs a route using the provided path and final hop constraints. -// Any destination specific fields from the final hop params will be attached -// assuming the destination's feature vector signals support, otherwise this -// method will fail. If the route is too long, or the selected path cannot -// support the fully payment including fees, then a non-nil error is returned. -// -// NOTE: The passed slice of ChannelHops MUST be sorted in forward order: from -// the source to the target node of the path finding attempt. It is assumed that -// any feature vectors on all hops have been validated for transitive -// dependencies. -func newRoute(sourceVertex route.Vertex, - pathEdges []*channeldb.CachedEdgePolicy, currentHeight uint32, - finalHop finalHopParams) (*route.Route, error) { - - var ( - hops []*route.Hop - - // totalTimeLock will accumulate the cumulative time lock - // across the entire route. This value represents how long the - // sender will need to wait in the *worst* case. - totalTimeLock = currentHeight - - // nextIncomingAmount is the amount that will need to flow into - // the *next* hop. Since we're going to be walking the route - // backwards below, this next hop gets closer and closer to the - // sender of the payment. - nextIncomingAmount lnwire.MilliSatoshi - ) - - pathLength := len(pathEdges) - for i := pathLength - 1; i >= 0; i-- { - // Now we'll start to calculate the items within the per-hop - // payload for the hop this edge is leading to. - edge := pathEdges[i] - - // We'll calculate the amounts, timelocks, and fees for each hop - // in the route. The base case is the final hop which includes - // their amount and timelocks. These values will accumulate - // contributions from the preceding hops back to the sender as - // we compute the route in reverse. - var ( - amtToForward lnwire.MilliSatoshi - fee lnwire.MilliSatoshi - outgoingTimeLock uint32 - tlvPayload bool - customRecords record.CustomSet - mpp *record.MPP - metadata []byte - ) - - // Define a helper function that checks this edge's feature - // vector for support for a given feature. We assume at this - // point that the feature vectors transitive dependencies have - // been validated. - supports := func(feature lnwire.FeatureBit) bool { - // If this edge comes from router hints, the features - // could be nil. - if edge.ToNodeFeatures == nil { - return false - } - return edge.ToNodeFeatures.HasFeature(feature) - } - - // We start by assuming the node doesn't support TLV. We'll now - // inspect the node's feature vector to see if we can promote - // the hop. We assume already that the feature vector's - // transitive dependencies have already been validated by path - // finding or some other means. - tlvPayload = supports(lnwire.TLVOnionPayloadOptional) - - if i == len(pathEdges)-1 { - // If this is the last hop, then the hop payload will - // contain the exact amount. In BOLT #4: Onion Routing - // Protocol / "Payload for the Last Node", this is - // detailed. - amtToForward = finalHop.amt - - // Fee is not part of the hop payload, but only used for - // reporting through RPC. Set to zero for the final hop. - fee = lnwire.MilliSatoshi(0) - - // As this is the last hop, we'll use the specified - // final CLTV delta value instead of the value from the - // last link in the route. - totalTimeLock += uint32(finalHop.cltvDelta) - outgoingTimeLock = totalTimeLock - - // Attach any custom records to the final hop if the - // receiver supports TLV. - if !tlvPayload && finalHop.records != nil { - return nil, errors.New("cannot attach " + - "custom records") - } - customRecords = finalHop.records - - // If we're attaching a payment addr but the receiver - // doesn't support both TLV and payment addrs, fail. - payAddr := supports(lnwire.PaymentAddrOptional) - if !payAddr && finalHop.paymentAddr != nil { - return nil, errors.New("cannot attach " + - "payment addr") - } - - // Otherwise attach the mpp record if it exists. - // TODO(halseth): move this to payment life cycle, - // where AMP options are set. - if finalHop.paymentAddr != nil { - mpp = record.NewMPP( - finalHop.totalAmt, - *finalHop.paymentAddr, - ) - } - - metadata = finalHop.metadata - } else { - // The amount that the current hop needs to forward is - // equal to the incoming amount of the next hop. - amtToForward = nextIncomingAmount - - // The fee that needs to be paid to the current hop is - // based on the amount that this hop needs to forward - // and its policy for the outgoing channel. This policy - // is stored as part of the incoming channel of - // the next hop. - fee = pathEdges[i+1].ComputeFee(amtToForward) - - // We'll take the total timelock of the preceding hop as - // the outgoing timelock or this hop. Then we'll - // increment the total timelock incurred by this hop. - outgoingTimeLock = totalTimeLock - totalTimeLock += uint32(pathEdges[i+1].TimeLockDelta) - } - - // Since we're traversing the path backwards atm, we prepend - // each new hop such that, the final slice of hops will be in - // the forwards order. - currentHop := &route.Hop{ - PubKeyBytes: edge.ToNodePubKey(), - ChannelID: edge.ChannelID, - AmtToForward: amtToForward, - OutgoingTimeLock: outgoingTimeLock, - LegacyPayload: !tlvPayload, - CustomRecords: customRecords, - MPP: mpp, - Metadata: metadata, - } - - hops = append([]*route.Hop{currentHop}, hops...) - - // Finally, we update the amount that needs to flow into the - // *next* hop, which is the amount this hop needs to forward, - // accounting for the fee that it takes. - nextIncomingAmount = amtToForward + fee - } - - // With the base routing data expressed as hops, build the full route - newRoute, err := route.NewRouteFromHops( - nextIncomingAmount, totalTimeLock, route.Vertex(sourceVertex), - hops, - ) - if err != nil { - return nil, err - } - - return newRoute, nil -} - // edgeWeight computes the weight of an edge. This value is used when searching // 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 diff --git a/routing/pathfind_test.go b/routing/pathfind_test.go index ce305d06ca..f449426dcc 100644 --- a/routing/pathfind_test.go +++ b/routing/pathfind_test.go @@ -1273,320 +1273,6 @@ func runPathFindingWithAdditionalEdges(t *testing.T, useCache bool) { assertExpectedPath(t, graph.aliasMap, path, "songoku", "doge") } -// TestNewRoute tests whether the construction of hop payloads by newRoute -// is executed correctly. -func TestNewRoute(t *testing.T) { - - var sourceKey [33]byte - sourceVertex := route.Vertex(sourceKey) - - testPaymentAddr := [32]byte{0x01, 0x02, 0x03} - - const ( - startingHeight = 100 - finalHopCLTV = 1 - ) - - createHop := func(baseFee lnwire.MilliSatoshi, - feeRate lnwire.MilliSatoshi, - bandwidth lnwire.MilliSatoshi, - timeLockDelta uint16) *channeldb.CachedEdgePolicy { - - return &channeldb.CachedEdgePolicy{ - ToNodePubKey: func() route.Vertex { - return route.Vertex{} - }, - ToNodeFeatures: lnwire.NewFeatureVector(nil, nil), - FeeProportionalMillionths: feeRate, - FeeBaseMSat: baseFee, - TimeLockDelta: timeLockDelta, - } - } - - testCases := []struct { - // name identifies the test case in the test output. - name string - - // hops is the list of hops (the route) that gets passed into - // the call to newRoute. - hops []*channeldb.CachedEdgePolicy - - // paymentAmount is the amount that is send into the route - // indicated by hops. - paymentAmount lnwire.MilliSatoshi - - // destFeatures is a feature vector, that if non-nil, will - // overwrite the final hop's feature vector in the graph. - destFeatures *lnwire.FeatureVector - - paymentAddr *[32]byte - - // metadata is the payment metadata to attach to the route. - metadata []byte - - // expectedFees is a list of fees that every hop is expected - // to charge for forwarding. - expectedFees []lnwire.MilliSatoshi - - // expectedTimeLocks is a list of time lock values that every - // hop is expected to specify in its outgoing HTLC. The time - // lock values in this list are relative to the current block - // height. - expectedTimeLocks []uint32 - - // expectedTotalAmount is the total amount that is expected to - // be returned from newRoute. This amount should include all - // the fees to be paid to intermediate hops. - expectedTotalAmount lnwire.MilliSatoshi - - // expectedTotalTimeLock is the time lock that is expected to - // be returned from newRoute. This is the time lock that should - // be specified in the HTLC that is sent by the source node. - // expectedTotalTimeLock is relative to the current block height. - expectedTotalTimeLock uint32 - - // expectError indicates whether the newRoute call is expected - // to fail or succeed. - expectError bool - - // expectedErrorCode indicates the expected error code when - // expectError is true. - expectedErrorCode errorCode - - expectedTLVPayload bool - - expectedMPP *record.MPP - }{ - { - // For a single hop payment, no fees are expected to be paid. - name: "single hop", - paymentAmount: 100000, - hops: []*channeldb.CachedEdgePolicy{ - createHop(100, 1000, 1000000, 10), - }, - metadata: []byte{1, 2, 3}, - expectedFees: []lnwire.MilliSatoshi{0}, - expectedTimeLocks: []uint32{1}, - expectedTotalAmount: 100000, - expectedTotalTimeLock: 1, - }, { - // For a two hop payment, only the fee for the first hop - // needs to be paid. The destination hop does not require - // a fee to receive the payment. - name: "two hop", - paymentAmount: 100000, - hops: []*channeldb.CachedEdgePolicy{ - createHop(0, 1000, 1000000, 10), - createHop(30, 1000, 1000000, 5), - }, - expectedFees: []lnwire.MilliSatoshi{130, 0}, - expectedTimeLocks: []uint32{1, 1}, - expectedTotalAmount: 100130, - expectedTotalTimeLock: 6, - }, { - // For a two hop payment, only the fee for the first hop - // needs to be paid. The destination hop does not require - // a fee to receive the payment. - name: "two hop tlv onion feature", - destFeatures: tlvFeatures, - paymentAmount: 100000, - hops: []*channeldb.CachedEdgePolicy{ - createHop(0, 1000, 1000000, 10), - createHop(30, 1000, 1000000, 5), - }, - expectedFees: []lnwire.MilliSatoshi{130, 0}, - expectedTimeLocks: []uint32{1, 1}, - expectedTotalAmount: 100130, - expectedTotalTimeLock: 6, - expectedTLVPayload: true, - }, { - // For a two hop payment, only the fee for the first hop - // needs to be paid. The destination hop does not require - // a fee to receive the payment. - name: "two hop single shot mpp", - destFeatures: tlvPayAddrFeatures, - paymentAddr: &testPaymentAddr, - paymentAmount: 100000, - hops: []*channeldb.CachedEdgePolicy{ - createHop(0, 1000, 1000000, 10), - createHop(30, 1000, 1000000, 5), - }, - expectedFees: []lnwire.MilliSatoshi{130, 0}, - expectedTimeLocks: []uint32{1, 1}, - expectedTotalAmount: 100130, - expectedTotalTimeLock: 6, - expectedTLVPayload: true, - expectedMPP: record.NewMPP( - 100000, testPaymentAddr, - ), - }, { - // A three hop payment where the first and second hop - // will both charge 1 msat. The fee for the first hop - // is actually slightly higher than 1, because the amount - // to forward also includes the fee for the second hop. This - // gets rounded down to 1. - name: "three hop", - paymentAmount: 100000, - hops: []*channeldb.CachedEdgePolicy{ - createHop(0, 10, 1000000, 10), - createHop(0, 10, 1000000, 5), - createHop(0, 10, 1000000, 3), - }, - expectedFees: []lnwire.MilliSatoshi{1, 1, 0}, - expectedTotalAmount: 100002, - expectedTimeLocks: []uint32{4, 1, 1}, - expectedTotalTimeLock: 9, - }, { - // A three hop payment where the fee of the first hop - // is slightly higher (11) than the fee at the second hop, - // because of the increase amount to forward. - name: "three hop with fee carry over", - paymentAmount: 100000, - hops: []*channeldb.CachedEdgePolicy{ - createHop(0, 10000, 1000000, 10), - createHop(0, 10000, 1000000, 5), - createHop(0, 10000, 1000000, 3), - }, - expectedFees: []lnwire.MilliSatoshi{1010, 1000, 0}, - expectedTotalAmount: 102010, - expectedTimeLocks: []uint32{4, 1, 1}, - expectedTotalTimeLock: 9, - }, { - // A three hop payment where the fee policies of the first and - // second hop are just high enough to show the fee carry over - // effect. - name: "three hop with minimal fees for carry over", - paymentAmount: 100000, - hops: []*channeldb.CachedEdgePolicy{ - createHop(0, 10000, 1000000, 10), - - // First hop charges 0.1% so the second hop fee - // should show up in the first hop fee as 1 msat - // extra. - createHop(0, 1000, 1000000, 5), - - // Second hop charges a fixed 1000 msat. - createHop(1000, 0, 1000000, 3), - }, - expectedFees: []lnwire.MilliSatoshi{101, 1000, 0}, - expectedTotalAmount: 101101, - expectedTimeLocks: []uint32{4, 1, 1}, - expectedTotalTimeLock: 9, - }} - - for _, testCase := range testCases { - testCase := testCase - - // Overwrite the final hop's features if the test requires a - // custom feature vector. - if testCase.destFeatures != nil { - finalHop := testCase.hops[len(testCase.hops)-1] - finalHop.ToNodeFeatures = testCase.destFeatures - } - - assertRoute := func(t *testing.T, route *route.Route) { - if route.TotalAmount != testCase.expectedTotalAmount { - t.Errorf("Expected total amount is be %v"+ - ", but got %v instead", - testCase.expectedTotalAmount, - route.TotalAmount) - } - - for i := 0; i < len(testCase.expectedFees); i++ { - fee := route.HopFee(i) - if testCase.expectedFees[i] != fee { - - t.Errorf("Expected fee for hop %v to "+ - "be %v, but got %v instead", - i, testCase.expectedFees[i], - fee) - } - } - - expectedTimeLockHeight := startingHeight + - testCase.expectedTotalTimeLock - - if route.TotalTimeLock != expectedTimeLockHeight { - - t.Errorf("Expected total time lock to be %v"+ - ", but got %v instead", - expectedTimeLockHeight, - route.TotalTimeLock) - } - - for i := 0; i < len(testCase.expectedTimeLocks); i++ { - expectedTimeLockHeight := startingHeight + - testCase.expectedTimeLocks[i] - - if expectedTimeLockHeight != - route.Hops[i].OutgoingTimeLock { - - t.Errorf("Expected time lock for hop "+ - "%v to be %v, but got %v instead", - i, expectedTimeLockHeight, - route.Hops[i].OutgoingTimeLock) - } - } - - finalHop := route.Hops[len(route.Hops)-1] - if !finalHop.LegacyPayload != - testCase.expectedTLVPayload { - - t.Errorf("Expected final hop tlv payload: %t, "+ - "but got: %t instead", - testCase.expectedTLVPayload, - !finalHop.LegacyPayload) - } - - if !reflect.DeepEqual( - finalHop.MPP, testCase.expectedMPP, - ) { - - t.Errorf("Expected final hop mpp field: %v, "+ - " but got: %v instead", - testCase.expectedMPP, finalHop.MPP) - } - - if !bytes.Equal(finalHop.Metadata, testCase.metadata) { - t.Errorf("Expected final metadata field: %v, "+ - " but got: %v instead", - testCase.metadata, finalHop.Metadata) - } - } - - t.Run(testCase.name, func(t *testing.T) { - route, err := newRoute( - sourceVertex, testCase.hops, startingHeight, - finalHopParams{ - amt: testCase.paymentAmount, - totalAmt: testCase.paymentAmount, - cltvDelta: finalHopCLTV, - records: nil, - paymentAddr: testCase.paymentAddr, - metadata: testCase.metadata, - }, - ) - - if testCase.expectError { - expectedCode := testCase.expectedErrorCode - if err == nil || !IsError(err, expectedCode) { - t.Fatalf("expected newRoute to fail "+ - "with error code %v but got "+ - "%v instead", - expectedCode, err) - } - } else { - if err != nil { - t.Errorf("unable to create path: %v", err) - return - } - - assertRoute(t, route) - } - }) - } -} - func runNewRoutePathTooLong(t *testing.T, useCache bool) { var testChannels []*testChannel