From 9b3b309f2db3b60bc35a4693c418e7270911cf97 Mon Sep 17 00:00:00 2001 From: bitromortac Date: Mon, 29 Jul 2024 11:31:24 +0200 Subject: [PATCH 1/4] routing: remove unused param, log --- routing/pathfind.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/routing/pathfind.go b/routing/pathfind.go index 086b81692..285d842e3 100644 --- a/routing/pathfind.go +++ b/routing/pathfind.go @@ -697,7 +697,6 @@ func findPath(g *graphParams, r *RestrictParams, cfg *PathFindingConfig, // processEdge is a helper closure that will be used to make sure edges // satisfy our specific requirements. processEdge := func(fromVertex route.Vertex, - fromFeatures *lnwire.FeatureVector, edge *unifiedEdge, toNodeDist *nodeWithDist) { edgesExpanded++ @@ -783,6 +782,17 @@ func findPath(g *graphParams, r *RestrictParams, cfg *PathFindingConfig, // Check if accumulated fees would exceed fee limit when this // node would be added to the path. totalFee := int64(netAmountToReceive) - int64(amt) + + log.Trace(lnutils.NewLogClosure(func() string { + return fmt.Sprintf( + "Checking fromVertex (%v) with "+ + "minInboundFee=%v, inboundFee=%v, "+ + "amountToSend=%v, amt=%v, totalFee=%v", + fromVertex, minInboundFee, inboundFee, + amountToSend, amt, totalFee, + ) + })) + if totalFee > 0 && lnwire.MilliSatoshi(totalFee) > r.FeeLimit { return } @@ -1035,7 +1045,7 @@ func findPath(g *graphParams, r *RestrictParams, cfg *PathFindingConfig, // Check if this candidate node is better than what we // already have. - processEdge(fromNode, fromFeatures, edge, partialPath) + processEdge(fromNode, edge, partialPath) } if nodeHeap.Len() == 0 { From 557b33733cf82c6a51c439a0b7ceb228443225c2 Mon Sep 17 00:00:00 2001 From: bitromortac Date: Fri, 26 Jul 2024 09:20:37 +0200 Subject: [PATCH 2/4] itest: add test for failing send after queryroutes --- itest/list_on_test.go | 4 ++ itest/lnd_routing_test.go | 78 +++++++++++++++++++++++++++++++++++++++ lntest/node/watcher.go | 2 +- 3 files changed, 83 insertions(+), 1 deletion(-) diff --git a/itest/list_on_test.go b/itest/list_on_test.go index 02fc3919f..c5d63bd2d 100644 --- a/itest/list_on_test.go +++ b/itest/list_on_test.go @@ -354,6 +354,10 @@ var allTestCases = []*lntest.TestCase{ Name: "route fee cutoff", TestFunc: testRouteFeeCutoff, }, + { + Name: "route fee limit after queryroutes", + TestFunc: testFeeLimitAfterQueryRoutes, + }, { Name: "rpc middleware interceptor", TestFunc: testRPCMiddlewareInterceptor, diff --git a/itest/lnd_routing_test.go b/itest/lnd_routing_test.go index 1e627c7d7..93dc23cab 100644 --- a/itest/lnd_routing_test.go +++ b/itest/lnd_routing_test.go @@ -1331,6 +1331,84 @@ func testRouteFeeCutoff(ht *lntest.HarnessTest) { ht.CloseChannel(carol, chanPointCarolDave) } +// testFeeLimitAfterQueryRoutes tests that a payment's fee limit is consistent +// with the fee of a queried route. +func testFeeLimitAfterQueryRoutes(ht *lntest.HarnessTest) { + // Create a three hop network: Alice -> Bob -> Carol. + chanAmt := btcutil.Amount(100000) + chanPoints, nodes := createSimpleNetwork( + ht, []string{}, 3, lntest.OpenChannelParams{Amt: chanAmt}, + ) + alice, bob, carol := nodes[0], nodes[1], nodes[2] + chanPointAliceBob, chanPointBobCarol := chanPoints[0], chanPoints[1] + + // We set an inbound fee discount on Bob's channel to Alice to + // effectively set the outbound fees charged to Carol to zero. + expectedPolicy := &lnrpc.RoutingPolicy{ + FeeBaseMsat: 1000, + FeeRateMilliMsat: 1, + InboundFeeBaseMsat: -1000, + InboundFeeRateMilliMsat: -1, + TimeLockDelta: uint32( + chainreg.DefaultBitcoinTimeLockDelta, + ), + MinHtlc: 1000, + MaxHtlcMsat: lntest.CalculateMaxHtlc(chanAmt), + } + + updateFeeReq := &lnrpc.PolicyUpdateRequest{ + Scope: &lnrpc.PolicyUpdateRequest_ChanPoint{ + ChanPoint: chanPointAliceBob, + }, + BaseFeeMsat: expectedPolicy.FeeBaseMsat, + FeeRatePpm: uint32(expectedPolicy.FeeRateMilliMsat), + TimeLockDelta: expectedPolicy.TimeLockDelta, + MaxHtlcMsat: expectedPolicy.MaxHtlcMsat, + InboundFee: &lnrpc.InboundFee{ + BaseFeeMsat: expectedPolicy.InboundFeeBaseMsat, + FeeRatePpm: expectedPolicy.InboundFeeRateMilliMsat, + }, + } + bob.RPC.UpdateChannelPolicy(updateFeeReq) + + // Wait for Alice to receive the channel update from Bob. + ht.AssertChannelPolicyUpdate( + alice, bob, expectedPolicy, chanPointAliceBob, false, + ) + + // We query the only route available to Carol. + queryRoutesReq := &lnrpc.QueryRoutesRequest{ + PubKey: carol.PubKeyStr, + Amt: paymentAmt, + } + routesResp := alice.RPC.QueryRoutes(queryRoutesReq) + + // Verify that the route has zero fees. + require.Len(ht, routesResp.Routes, 1) + require.Len(ht, routesResp.Routes[0].Hops, 2) + require.Zero(ht, routesResp.Routes[0].TotalFeesMsat) + + // Attempt a payment with a fee limit of zero. + invoice := &lnrpc.Invoice{Value: paymentAmt} + invoiceResp := carol.RPC.AddInvoice(invoice) + sendReq := &routerrpc.SendPaymentRequest{ + PaymentRequest: invoiceResp.PaymentRequest, + TimeoutSeconds: 60, + FeeLimitMsat: 0, + } + + // We assert that the payment fails because the fee limit doesn't work + // correctly. This is fixed in the next commit. + ht.SendPaymentAssertFail( + alice, sendReq, + lnrpc.PaymentFailureReason_FAILURE_REASON_NO_ROUTE, + ) + + // Once we're done, close the channels. + ht.CloseChannel(alice, chanPointAliceBob) + ht.CloseChannel(bob, chanPointBobCarol) +} + // computeFee calculates the payment fee as specified in BOLT07. func computeFee(baseFee, feeRate, amt lnwire.MilliSatoshi) lnwire.MilliSatoshi { return baseFee + amt*feeRate/1000000 diff --git a/lntest/node/watcher.go b/lntest/node/watcher.go index 7656cb3c5..f7672b825 100644 --- a/lntest/node/watcher.go +++ b/lntest/node/watcher.go @@ -221,7 +221,7 @@ func (nw *nodeWatcher) WaitForChannelPolicyUpdate( select { // Send a watch request every second. case <-ticker.C: - // Did the event can close in the meantime? We want to + // Did the event chan close in the meantime? We want to // avoid a "close of closed channel" panic since we're // re-using the same event chan for multiple requests. select { From b0f0715813977ec4a7bc63e0e3bc2d01bf43bb27 Mon Sep 17 00:00:00 2001 From: bitromortac Date: Fri, 26 Jul 2024 11:17:00 +0200 Subject: [PATCH 3/4] routing: fix fee limit condition When iterating edges, pathfinding checks early whether using an edge would violate the requested total fee limit for a route. This check is done on the net amount (an amount the inbound fee is calculated with). However, a possible next hop's fee discount leads to a reduction in fees and as such using the net amount leads to assuming a higher cumulative fee than the route really has, excluding the path erroneously. We perform the fee limit check on the amount to send, which includes both inbound and outbound fees. This should be possible as the first hop's outbound fee is zero and therefore doesn't have to be checked in the end. --- docs/release-notes/release-notes-0.18.3.md | 5 +++ itest/lnd_routing_test.go | 8 ++--- routing/pathfind.go | 36 +++++++++++----------- 3 files changed, 25 insertions(+), 24 deletions(-) diff --git a/docs/release-notes/release-notes-0.18.3.md b/docs/release-notes/release-notes-0.18.3.md index 4cb59b76d..6672a6e58 100644 --- a/docs/release-notes/release-notes-0.18.3.md +++ b/docs/release-notes/release-notes-0.18.3.md @@ -36,6 +36,10 @@ * [Fixed a bug](https://github.com/lightningnetwork/lnd/pull/8896) that caused LND to use a default fee rate for the batch channel opening flow. +* The fee limit for payments [was made + compatible](https://github.com/lightningnetwork/lnd/pull/8941) with inbound + fees. + # New Features ## Functional Enhancements ## RPC Additions @@ -150,6 +154,7 @@ # Contributors (Alphabetical Order) * Andras Banki-Horvath +* bitromortac * Bufo * Elle Mouton * Matheus Degiovani diff --git a/itest/lnd_routing_test.go b/itest/lnd_routing_test.go index 93dc23cab..0b1cfebe8 100644 --- a/itest/lnd_routing_test.go +++ b/itest/lnd_routing_test.go @@ -1397,12 +1397,8 @@ func testFeeLimitAfterQueryRoutes(ht *lntest.HarnessTest) { FeeLimitMsat: 0, } - // We assert that the payment fails because the fee limit doesn't work - // correctly. This is fixed in the next commit. - ht.SendPaymentAssertFail( - alice, sendReq, - lnrpc.PaymentFailureReason_FAILURE_REASON_NO_ROUTE, - ) + // We assert that a route compatible with the fee limit is available. + ht.SendPaymentAssertSettled(alice, sendReq) // Once we're done, close the channels. ht.CloseChannel(alice, chanPointAliceBob) diff --git a/routing/pathfind.go b/routing/pathfind.go index 285d842e3..652491763 100644 --- a/routing/pathfind.go +++ b/routing/pathfind.go @@ -723,6 +723,24 @@ func findPath(g *graphParams, r *RestrictParams, cfg *PathFindingConfig, amountToSend := toNodeDist.netAmountReceived + lnwire.MilliSatoshi(inboundFee) + // Check if accumulated fees would exceed fee limit when this + // node would be added to the path. + totalFee := int64(amountToSend) - int64(amt) + + log.Trace(lnutils.NewLogClosure(func() string { + return fmt.Sprintf( + "Checking fromVertex (%v) with "+ + "minInboundFee=%v, inboundFee=%v, "+ + "amountToSend=%v, amt=%v, totalFee=%v", + fromVertex, minInboundFee, inboundFee, + amountToSend, amt, totalFee, + ) + })) + + if totalFee > 0 && lnwire.MilliSatoshi(totalFee) > r.FeeLimit { + return + } + // Request the success probability for this edge. edgeProbability := r.ProbabilitySource( fromVertex, toNodeDist.node, amountToSend, @@ -779,24 +797,6 @@ func findPath(g *graphParams, r *RestrictParams, cfg *PathFindingConfig, netAmountToReceive := amountToSend + lnwire.MilliSatoshi(outboundFee) - // Check if accumulated fees would exceed fee limit when this - // node would be added to the path. - totalFee := int64(netAmountToReceive) - int64(amt) - - log.Trace(lnutils.NewLogClosure(func() string { - return fmt.Sprintf( - "Checking fromVertex (%v) with "+ - "minInboundFee=%v, inboundFee=%v, "+ - "amountToSend=%v, amt=%v, totalFee=%v", - fromVertex, minInboundFee, inboundFee, - amountToSend, amt, totalFee, - ) - })) - - if totalFee > 0 && lnwire.MilliSatoshi(totalFee) > r.FeeLimit { - return - } - // Calculate total probability of successfully reaching target // by multiplying the probabilities. Both this edge and the rest // of the route must succeed. From 0358b3a0fa0f34cecf4173eb8a0e8e969e5df9c7 Mon Sep 17 00:00:00 2001 From: bitromortac Date: Mon, 29 Jul 2024 11:12:55 +0200 Subject: [PATCH 4/4] routing: use amount to send for time lock weight The time lock weight for a hop is supposed to be proportional to the amount that is sent/locked, but in a previous change we switched to the net amount, where inbound fees aren't yet applied. This is corrected in this commit. --- routing/pathfind.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routing/pathfind.go b/routing/pathfind.go index 652491763..d76c5ea22 100644 --- a/routing/pathfind.go +++ b/routing/pathfind.go @@ -823,7 +823,7 @@ func findPath(g *graphParams, r *RestrictParams, cfg *PathFindingConfig, // weight composed of the fee that this node will charge and // the amount that will be locked for timeLockDelta blocks in // the HTLC that is handed out to fromVertex. - weight := edgeWeight(netAmountToReceive, fee, timeLockDelta) + weight := edgeWeight(amountToSend, fee, timeLockDelta) // Compute the tentative weight to this new channel/edge // which is the weight from our toNode to the target node