routing: fix fee limit condition #8941

bitromortac · 2024-07-26T10:06:53Z

Change Description

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. With this PR, we perform the fee limit check on the amount to send, which includes both inbound and outbound fees, to fix #8940.

Todo:

remove itest, add more lightweight test?

coderabbitai · 2024-07-26T10:06:59Z

Important

Review skipped

Auto reviews are limited to specific labels.

Labels to auto review (1)

llm-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

yyforyongyu

Thanks for the quick fix! Just a few nits, otherwise LGTM.

remove itest, add more lightweight test?

We do test inbound fees in testMultiHopPayments, but the FeeLimitMsat behavior was never tested there. We then have testRouteFeeCutoff which I think it's a bit difficult to add this test as it complicates things. So I'd prefer we keep the newly added test as it's straightforward.

A unit test can also be helpful, tho I'd say it's better to refactor the extremely long method findPath first.

Given this is a fairly small change and a bug fix, I think we can still make it into 0.18.3? cc @saubyk

yyforyongyu · 2024-07-26T11:42:07Z

itest/lnd_routing_test.go

+func testFeeLimitAfterQueryRoutes(ht *lntest.HarnessTest) {
+	// For this test, we'll create the following topology:
+	//
+	// Alice ---- Bob ---- Carol


nit: there's this newer method you can use to quickly setup a topology,

// Create a three hop network: Alice -> Bob -> Carol. chanPoints, nodes := createSimpleNetwork(ht, cfg, 3, openChannelParams)

yyforyongyu · 2024-07-26T12:06:43Z

routing/pathfind.go

@@ -722,6 +722,13 @@ 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)


I find this log helpful when checking the behavior from the itest,

log.Tracef("Checking toNode(%v) with minInboundFee=%v, inboundFee=%v, amountToSend=%v, amt=%v, totalFee=%v", toNodeDist.node, minInboundFee, inboundFee, amountToSend, amt, totalFee)

+1 for the trace log, would be helpful when debugging inbound fees.

I added the trace log to be visible in both itest cases (without and with the fix)

bhandras

Thanks for the super quick fix, LGTM, peding @yyforyongyu's comments 🎉

bhandras · 2024-07-26T12:44:22Z

routing/pathfind.go

@@ -722,6 +722,13 @@ 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)


+1 for the trace log, would be helpful when debugging inbound fees.

bitromortac · 2024-07-26T13:37:02Z

routing/pathfind.go

+		if totalFee > 0 && lnwire.MilliSatoshi(totalFee) > r.FeeLimit {
+			return
+		}
+
 		// Request the success probability for this edge.
 		edgeProbability := r.ProbabilitySource(
 			fromVertex, toNodeDist.node, amountToSend,


I think we should also correct weight := edgeWeight(netAmountToReceive, fee, timeLockDelta) to weight := edgeWeight(amountToSend, fee, timeLockDelta)

in that case it should be weight := edgeWeight(amountToSend, outboundFee, timeLockDelta)?

the fee is just passed in there to be converted to int64, but isn't really related to the time lock weight, which is a bit strange and can be addressed in a refactor, so I think weight := edgeWeight(amountToSend, fee, timeLockDelta) is correct (and otherwise we wouldn't take inbound fees into account in the weight if fee would be changed to outboundFee).

bitromortac · 2024-07-29T10:34:42Z

Thanks for the quick reviews! I added some logging and a fix for the time lock weight.

Strange this doesn't compile with Error: routing/pathfind.go:729:13: undefined: newLogClosure, locally it works

yyforyongyu

Needs a rebase to use the new log closure, otherwise LGTM.

yyforyongyu · 2024-07-29T12:05:25Z

routing/pathfind.go

+		// node would be added to the path.
+		totalFee := int64(amountToSend) - int64(amt)
+
+		log.Trace(newLogClosure(func() string {


This is now lnutils.NewLogClosure

that was very strange, I could build the same commit on two different machines, maybe a github caching thing

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.

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.

yyforyongyu

LGTM🎉

yyforyongyu reviewed Jul 26, 2024

View reviewed changes

bhandras approved these changes Jul 26, 2024

View reviewed changes

bitromortac commented Jul 26, 2024

View reviewed changes

saubyk added this to the v0.18.3 milestone Jul 26, 2024

feelancer21 mentioned this pull request Jul 28, 2024

[feature]: Proposal for a different cutoff between InboundFee and OutboundFee in pathfinding #8945

Open

bitromortac force-pushed the fee-limit-inbound branch 3 times, most recently from d53a038 to b737bca Compare July 29, 2024 10:28

bitromortac requested a review from yyforyongyu July 29, 2024 10:28

yyforyongyu reviewed Jul 29, 2024

View reviewed changes

saubyk assigned bitromortac Jul 29, 2024

bitromortac added 4 commits July 30, 2024 08:52

routing: remove unused param, log

9b3b309

itest: add test for failing send after queryroutes

557b337

routing: use amount to send for time lock weight

0358b3a

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.

bitromortac force-pushed the fee-limit-inbound branch from b737bca to 0358b3a Compare July 30, 2024 06:57

bitromortac requested a review from yyforyongyu July 30, 2024 14:15

yyforyongyu approved these changes Jul 30, 2024

View reviewed changes

guggero merged commit 7f9fbbe into lightningnetwork:master Jul 30, 2024
30 of 34 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

routing: fix fee limit condition #8941

routing: fix fee limit condition #8941

bitromortac commented Jul 26, 2024

coderabbitai bot commented Jul 26, 2024

Review skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

yyforyongyu left a comment

yyforyongyu Jul 26, 2024

yyforyongyu Jul 26, 2024

bhandras Jul 26, 2024

bitromortac Jul 29, 2024

bhandras left a comment

bhandras Jul 26, 2024

bitromortac Jul 26, 2024

yyforyongyu Jul 26, 2024

bitromortac Jul 29, 2024 •

edited

Loading

bitromortac commented Jul 29, 2024

yyforyongyu left a comment

yyforyongyu Jul 29, 2024

bitromortac Jul 30, 2024

yyforyongyu left a comment

routing: fix fee limit condition #8941

routing: fix fee limit condition #8941

Conversation

bitromortac commented Jul 26, 2024

Change Description

coderabbitai bot commented Jul 26, 2024

Review skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhandras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitromortac Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

bitromortac commented Jul 29, 2024

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

CodeRabbit Configuration File (`.coderabbit.yaml`)

bitromortac Jul 29, 2024 •

edited

Loading