From 68d022956da22f7d97dca2f2617fbb1a7a87eb43 Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Tue, 13 Sep 2022 11:45:58 +0200 Subject: [PATCH 1/5] routing/test: remove unused field --- routing/pathfind_test.go | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/routing/pathfind_test.go b/routing/pathfind_test.go index 7f576196a4..4298ba1fe9 100644 --- a/routing/pathfind_test.go +++ b/routing/pathfind_test.go @@ -1306,7 +1306,6 @@ func TestNewRoute(t *testing.T) { createHop := func(baseFee lnwire.MilliSatoshi, feeRate lnwire.MilliSatoshi, - bandwidth lnwire.MilliSatoshi, timeLockDelta uint16) *channeldb.CachedEdgePolicy { return &channeldb.CachedEdgePolicy{ @@ -1379,7 +1378,7 @@ func TestNewRoute(t *testing.T) { name: "single hop", paymentAmount: 100000, hops: []*channeldb.CachedEdgePolicy{ - createHop(100, 1000, 1000000, 10), + createHop(100, 1000, 10), }, metadata: []byte{1, 2, 3}, expectedFees: []lnwire.MilliSatoshi{0}, @@ -1393,8 +1392,8 @@ func TestNewRoute(t *testing.T) { name: "two hop", paymentAmount: 100000, hops: []*channeldb.CachedEdgePolicy{ - createHop(0, 1000, 1000000, 10), - createHop(30, 1000, 1000000, 5), + createHop(0, 1000, 10), + createHop(30, 1000, 5), }, expectedFees: []lnwire.MilliSatoshi{130, 0}, expectedTimeLocks: []uint32{1, 1}, @@ -1408,8 +1407,8 @@ func TestNewRoute(t *testing.T) { destFeatures: tlvFeatures, paymentAmount: 100000, hops: []*channeldb.CachedEdgePolicy{ - createHop(0, 1000, 1000000, 10), - createHop(30, 1000, 1000000, 5), + createHop(0, 1000, 10), + createHop(30, 1000, 5), }, expectedFees: []lnwire.MilliSatoshi{130, 0}, expectedTimeLocks: []uint32{1, 1}, @@ -1425,8 +1424,8 @@ func TestNewRoute(t *testing.T) { paymentAddr: &testPaymentAddr, paymentAmount: 100000, hops: []*channeldb.CachedEdgePolicy{ - createHop(0, 1000, 1000000, 10), - createHop(30, 1000, 1000000, 5), + createHop(0, 1000, 10), + createHop(30, 1000, 5), }, expectedFees: []lnwire.MilliSatoshi{130, 0}, expectedTimeLocks: []uint32{1, 1}, @@ -1445,9 +1444,9 @@ func TestNewRoute(t *testing.T) { name: "three hop", paymentAmount: 100000, hops: []*channeldb.CachedEdgePolicy{ - createHop(0, 10, 1000000, 10), - createHop(0, 10, 1000000, 5), - createHop(0, 10, 1000000, 3), + createHop(0, 10, 10), + createHop(0, 10, 5), + createHop(0, 10, 3), }, expectedFees: []lnwire.MilliSatoshi{1, 1, 0}, expectedTotalAmount: 100002, @@ -1460,9 +1459,9 @@ func TestNewRoute(t *testing.T) { 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), + createHop(0, 10000, 10), + createHop(0, 10000, 5), + createHop(0, 10000, 3), }, expectedFees: []lnwire.MilliSatoshi{1010, 1000, 0}, expectedTotalAmount: 102010, @@ -1475,15 +1474,15 @@ func TestNewRoute(t *testing.T) { name: "three hop with minimal fees for carry over", paymentAmount: 100000, hops: []*channeldb.CachedEdgePolicy{ - createHop(0, 10000, 1000000, 10), + createHop(0, 10000, 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), + createHop(0, 1000, 5), // Second hop charges a fixed 1000 msat. - createHop(1000, 0, 1000000, 3), + createHop(1000, 0, 3), }, expectedFees: []lnwire.MilliSatoshi{101, 1000, 0}, expectedTotalAmount: 101101, From d6a54226b15e32f87c52c0b4b47036f3d6beb87e Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Tue, 13 Sep 2022 12:18:39 +0200 Subject: [PATCH 2/5] routing: apply min htlc in newRoute fix min htlc newroute --- routing/pathfind.go | 31 ++++++++++++++-------- routing/pathfind_test.go | 55 +++++++++++++++++++++++++++++++++++++--- 2 files changed, 73 insertions(+), 13 deletions(-) diff --git a/routing/pathfind.go b/routing/pathfind.go index a92d0a7e52..47139abad5 100644 --- a/routing/pathfind.go +++ b/routing/pathfind.go @@ -137,7 +137,6 @@ func newRoute(sourceVertex route.Vertex, // we compute the route in reverse. var ( amtToForward lnwire.MilliSatoshi - fee lnwire.MilliSatoshi outgoingTimeLock uint32 tlvPayload bool customRecords record.CustomSet @@ -172,9 +171,15 @@ func newRoute(sourceVertex route.Vertex, // 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) + // If this amount is however below the edge's min htlc + // constraint, then raise the amount to that minimum. A + // difference between the amount in the onion payload + // and the actual htlc amount is not tolerated. + if amtToForward < edge.MinHTLC { + amtToForward = edge.MinHTLC + } + + nextIncomingAmount = amtToForward // As this is the last hop, we'll use the specified // final CLTV delta value instead of the value from the @@ -219,7 +224,18 @@ func newRoute(sourceVertex route.Vertex, // 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) + fee := pathEdges[i+1].ComputeFee(amtToForward) + + // 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 + + // If this amount is below the edge's min htlc + // constraint, then raise the amount to that minimum. + if nextIncomingAmount < edge.MinHTLC { + nextIncomingAmount = edge.MinHTLC + } // We'll take the total timelock of the preceding hop as // the outgoing timelock or this hop. Then we'll @@ -243,11 +259,6 @@ func newRoute(sourceVertex route.Vertex, } 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 diff --git a/routing/pathfind_test.go b/routing/pathfind_test.go index 4298ba1fe9..19a4284dec 100644 --- a/routing/pathfind_test.go +++ b/routing/pathfind_test.go @@ -1304,11 +1304,21 @@ func TestNewRoute(t *testing.T) { finalHopCLTV = 1 ) + type createOpt func(*channeldb.CachedEdgePolicy) + + withHtlcAmtRange := func(min, max lnwire.MilliSatoshi) createOpt { + return func(o *channeldb.CachedEdgePolicy) { + o.MinHTLC = min + o.MaxHTLC = max + o.MessageFlags |= lnwire.ChanUpdateOptionMaxHtlc + } + } + createHop := func(baseFee lnwire.MilliSatoshi, feeRate lnwire.MilliSatoshi, - timeLockDelta uint16) *channeldb.CachedEdgePolicy { + timeLockDelta uint16, opts ...createOpt) *channeldb.CachedEdgePolicy { - return &channeldb.CachedEdgePolicy{ + policy := &channeldb.CachedEdgePolicy{ ToNodePubKey: func() route.Vertex { return route.Vertex{} }, @@ -1317,6 +1327,12 @@ func TestNewRoute(t *testing.T) { FeeBaseMSat: baseFee, TimeLockDelta: timeLockDelta, } + + for _, o := range opts { + o(policy) + } + + return policy } testCases := []struct { @@ -1488,7 +1504,40 @@ func TestNewRoute(t *testing.T) { expectedTotalAmount: 101101, expectedTimeLocks: []uint32{4, 1, 1}, expectedTotalTimeLock: 9, - }} + }, + { + name: "min htlc", + paymentAmount: 4000, + hops: []*channeldb.CachedEdgePolicy{ + createHop( + 0, 0, 10, + ), + createHop( + 4000, 20000, 10, + withHtlcAmtRange(10000, 50000), + ), + createHop( + 3000, 10000, 10, + withHtlcAmtRange(5000, 50000), + ), + }, + expectedTotalAmount: 14200, + expectedTotalTimeLock: 21, + expectedFees: []lnwire.MilliSatoshi{ + // First hop fee is based on min htlc of + // outgoing channel. + 4200, + + // Second hop gets overpaid fee because of min + // htlc. + 5000, + + // Final hop gets paid no fee, but is overpaid + // the payment. + 0, + }, + }, + } for _, testCase := range testCases { testCase := testCase From 5155b89bf6d90fbdf4f597216367c929be1584cf Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Tue, 13 Sep 2022 12:27:19 +0200 Subject: [PATCH 3/5] routing/test: simplify expected error check --- routing/pathfind_test.go | 23 ++++------------------- 1 file changed, 4 insertions(+), 19 deletions(-) diff --git a/routing/pathfind_test.go b/routing/pathfind_test.go index 19a4284dec..816f500227 100644 --- a/routing/pathfind_test.go +++ b/routing/pathfind_test.go @@ -1377,13 +1377,9 @@ func TestNewRoute(t *testing.T) { // expectedTotalTimeLock is relative to the current block height. expectedTotalTimeLock uint32 - // expectError indicates whether the newRoute call is expected + // expectedError 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 + expectedError error expectedTLVPayload bool @@ -1632,20 +1628,9 @@ func TestNewRoute(t *testing.T) { }, ) - 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 - } + require.ErrorIs(t, err, testCase.expectedError) + if testCase.expectedError == nil { assertRoute(t, route) } }) From 3df0ff9c9957448ecd6a49742f7a7826e5ab87ae Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Tue, 13 Sep 2022 12:31:34 +0200 Subject: [PATCH 4/5] routing: add max htlc check to newRoute Sanity check to prevent invalid routes from being constructed. --- routing/pathfind.go | 7 +++++++ routing/pathfind_test.go | 15 +++++++++++++++ routing/route/route.go | 4 ++++ 3 files changed, 26 insertions(+) diff --git a/routing/pathfind.go b/routing/pathfind.go index 47139abad5..4f7bd32480 100644 --- a/routing/pathfind.go +++ b/routing/pathfind.go @@ -259,6 +259,13 @@ func newRoute(sourceVertex route.Vertex, } hops = append([]*route.Hop{currentHop}, hops...) + + // Fail route building if max htlc is exceeded. + if edge.MessageFlags.HasMaxHtlc() && + nextIncomingAmount > edge.MaxHTLC { + + return nil, route.ErrMaxHtlcExceeded + } } // With the base routing data expressed as hops, build the full route diff --git a/routing/pathfind_test.go b/routing/pathfind_test.go index 816f500227..0951920525 100644 --- a/routing/pathfind_test.go +++ b/routing/pathfind_test.go @@ -1533,6 +1533,21 @@ func TestNewRoute(t *testing.T) { 0, }, }, + { + name: "max htlc", + paymentAmount: 4000, + hops: []*channeldb.CachedEdgePolicy{ + createHop( + 0, 0, 10, + withHtlcAmtRange(0, 8000), + ), + createHop( + 3000, 10000, 10, + withHtlcAmtRange(5000, 10000), + ), + }, + expectedError: route.ErrMaxHtlcExceeded, + }, } for _, testCase := range testCases { diff --git a/routing/route/route.go b/routing/route/route.go index 5992bd4046..8104d7bb0f 100644 --- a/routing/route/route.go +++ b/routing/route/route.go @@ -30,6 +30,10 @@ var ( // construct a new sphinx packet, but provides too many hops. ErrMaxRouteHopsExceeded = fmt.Errorf("route has too many hops") + // ErrMaxHtlcExceeded is returned when a channel policy's max htlc is + // exceeded. + ErrMaxHtlcExceeded = fmt.Errorf("route exceeds max htlc policy") + // ErrIntermediateMPPHop is returned when a hop tries to deliver an MPP // record to an intermediate hop, only final hops can receive MPP // records. From 4f6cde98a3908fdd763e5c2abe4d7c07eb1e1cba Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Tue, 13 Sep 2022 14:05:01 +0200 Subject: [PATCH 5/5] routing: simplify BuildRoute With newRoute being aware of min_htlc constraints, there is no more need to do a forward pass in BuildRoute. This also fixes potential edge cases where the forward pass gets to a payment amount that still violates htlc ranges. The downside of this simplification is that more value may be dropped off along the path as fees rather than paying extra to the final hop. However, as the primary intention of the BuildRoute `min_amt` flag is to allow for cheap probes, this change is likely to have no consequences. --- channeldb/graph.go | 16 ------------- channeldb/graph_cache.go | 11 --------- channeldb/graph_test.go | 5 ---- routing/router.go | 52 ++++++++++------------------------------ 4 files changed, 13 insertions(+), 71 deletions(-) diff --git a/channeldb/graph.go b/channeldb/graph.go index 957c0b828d..084c2f77f8 100644 --- a/channeldb/graph.go +++ b/channeldb/graph.go @@ -3489,22 +3489,6 @@ func (c *ChannelEdgePolicy) ComputeFee( return c.FeeBaseMSat + (amt*c.FeeProportionalMillionths)/feeRateParts } -// divideCeil divides dividend by factor and rounds the result up. -func divideCeil(dividend, factor lnwire.MilliSatoshi) lnwire.MilliSatoshi { - return (dividend + factor - 1) / factor -} - -// ComputeFeeFromIncoming computes the fee to forward an HTLC given the incoming -// amount. -func (c *ChannelEdgePolicy) ComputeFeeFromIncoming( - incomingAmt lnwire.MilliSatoshi) lnwire.MilliSatoshi { - - return incomingAmt - divideCeil( - feeRateParts*(incomingAmt-c.FeeBaseMSat), - feeRateParts+c.FeeProportionalMillionths, - ) -} - // FetchChannelEdgesByOutpoint attempts to lookup the two directed edges for // the channel identified by the funding outpoint. If the channel can't be // found, then ErrEdgeNotFound is returned. A struct which houses the general diff --git a/channeldb/graph_cache.go b/channeldb/graph_cache.go index 1aae21d06d..9ef9ef71d5 100644 --- a/channeldb/graph_cache.go +++ b/channeldb/graph_cache.go @@ -92,17 +92,6 @@ func (c *CachedEdgePolicy) ComputeFee( return c.FeeBaseMSat + (amt*c.FeeProportionalMillionths)/feeRateParts } -// ComputeFeeFromIncoming computes the fee to forward an HTLC given the incoming -// amount. -func (c *CachedEdgePolicy) ComputeFeeFromIncoming( - incomingAmt lnwire.MilliSatoshi) lnwire.MilliSatoshi { - - return incomingAmt - divideCeil( - feeRateParts*(incomingAmt-c.FeeBaseMSat), - feeRateParts+c.FeeProportionalMillionths, - ) -} - // NewCachedPolicy turns a full policy into a minimal one that can be cached. func NewCachedPolicy(policy *ChannelEdgePolicy) *CachedEdgePolicy { return &CachedEdgePolicy{ diff --git a/channeldb/graph_test.go b/channeldb/graph_test.go index 35c7050eb3..17b32c8946 100644 --- a/channeldb/graph_test.go +++ b/channeldb/graph_test.go @@ -3264,11 +3264,6 @@ func TestComputeFee(t *testing.T) { if fee != expectedFee { t.Fatalf("expected fee %v, got %v", expectedFee, fee) } - - fwdFee := policy.ComputeFeeFromIncoming(outgoingAmt + fee) - if fwdFee != expectedFee { - t.Fatalf("expected fee %v, but got %v", fee, fwdFee) - } } // TestBatchedAddChannelEdge asserts that BatchedAddChannelEdge properly diff --git a/routing/router.go b/routing/router.go index 348914db73..8171a21a7f 100644 --- a/routing/router.go +++ b/routing/router.go @@ -2731,22 +2731,23 @@ func (r *ChannelRouter) BuildRoute(amt *lnwire.MilliSatoshi, // Allocate a list that will contain the unified policies for this // route. - edges := make([]*unifiedPolicy, len(hops)) + edges := make([]*channeldb.CachedEdgePolicy, len(hops)) - var runningAmt lnwire.MilliSatoshi + var paymentAmt 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 // min_htlc channel policy of zero, which could lead to a zero // amount payment being made. - runningAmt = 1 + paymentAmt = 1 } else { // If an amount is specified, we need to build a route that // delivers exactly this amount to the final destination. - runningAmt = *amt + paymentAmt = *amt } // Traverse hops backwards to accumulate fees in the running amounts. + runningAmt := paymentAmt source := r.selfNode.PubKeyBytes for i := len(hops) - 1; i >= 0; i-- { toNode := hops[i] @@ -2778,12 +2779,10 @@ func (r *ChannelRouter) BuildRoute(amt *lnwire.MilliSatoshi, } } - // If using min amt, increase amt if needed. - if useMinAmt { - min := unifiedPolicy.minAmt() - if min > runningAmt { - runningAmt = min - } + // Increase amount if needed to satisfy minimum htlc size. + min := unifiedPolicy.minAmt() + if min > runningAmt { + runningAmt = min } // Get a forwarding policy for the specific amount that we want @@ -2803,40 +2802,15 @@ func (r *ChannelRouter) BuildRoute(amt *lnwire.MilliSatoshi, log.Tracef("Select channel %v at position %v", policy.ChannelID, i) - edges[i] = unifiedPolicy - } - - // 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, - } - } - - if i > 0 { - // Decrease the amount to send while going forward. - receiverAmt -= policy.ComputeFeeFromIncoming( - receiverAmt, - ) - } - - pathEdges = append(pathEdges, policy) + edges[i] = policy } // Build and return the final route. return newRoute( - source, pathEdges, uint32(height), + source, edges, uint32(height), finalHopParams{ - amt: receiverAmt, - totalAmt: receiverAmt, + amt: paymentAmt, + totalAmt: paymentAmt, cltvDelta: uint16(finalCltvDelta), records: nil, paymentAddr: payAddr,