routing: inbound fees support for BuildRoute #8886

bitromortac · 2024-07-02T11:41:31Z

This PR attempts to add support for inbound fees to BuildRoute. The three passes BuildRoute goes through (see #8814 (comment) for an explanation) are altered to determine the edges in the backward pass (from receiver to sender) as opposed to the previous approach where this is done in the forward pass (sender to receiver). This way we get fixed edges that can be used to determine the minimal sender/maximal receiver amount with inbound fees, doing a reverse amount calculation (thanks to @feelancer21 for the analysis #8814 (comment)). The computation for the outgoing amount based on the incoming amount is done using big.Int arithmetic.

Previous work on this: #7060

coderabbitai · 2024-07-02T11:41:37Z

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.

ProofOfKeags

So this is my first time looking at this corner of the codebase, so the quality of my feedback may not be all that great but here is what I've come to understand.

The algorithm seems to be intrinsically imperative and essentially must be computed iteratively. Normally I hate this but in order to satisfy MinHTLC requirements I don't think there is a way around that. Given that the algorithm must be imperative, I think it is extremely important to be specific about the role of each variable. Unfortunately I'm not completely clear on precisely how we are computing things. My understanding is that there are broadly two cases that we need to consider:

We want to solve for the smallest amount we need to send to allow the receiver to successfully receive 1 sat.
We want to compute the amount that we need to send in order for the receiver to receive some other amount ignoring MinHTLC requirements. NOTE: I find it odd that we ignore MinHTLC requirements in this second case but it is a consequence of how the code is currently written, it's possible this wasn't intended and if so should be addressed.

From here It comes to my attention that the following expressions should hold:

sendAmt := recvAmt + fees
fees := outboundFees + inboundFees
outboundFees := fn.Sum(fn.Map(getOutboundFee, edges[1:]))
inboundFees := fn.Sum(fn.Map(getInboundFee, edges[:len(edges)-1]))

Separately, I have a few notes about naming things. Functions should be named according to the properties they satisfy, not based off of the procedure they follow. This is because the caller does not care about how a computation is carried out, they care about what computation is being done. Similarly, for variables, I'd recommend (though I do not insist on) naming them according to the eventual result they will produce in the case of accumulators, and for purely transient variables, you can drop things like "next" or "temp" and just favor using your character budget on being specific about what value is under that iterator.

I understand that a substantial portion of what I'm commenting on precedes this PR, however it is hard for me to comment on the quality of the diff without implicitly commenting on the quality of the existing codebase. Improving some of these things may be beyond the desired scope of this PR and in that case it's OK. However, in other parts of LND I've been favoring the approach of "no PR should make the quality of the codebase deteriorate".

Lmk if you think this is reasonable and attainable. If not, maybe we can converge on something that improves the codebase incrementally while not distracting us too much from the main feature goal.

routing/router.go

guggero

Did a first pass to load context. Refactor work looks good to me 👍
But looks like the actual calculation has some TODOs left, so will revisit once ready.

routing/router.go

guggero · 2024-07-24T12:22:24Z

docs/release-notes/release-notes-0.18.2.md

@@ -59,6 +59,9 @@
 * [`ChanInfoRequest`](https://github.com/lightningnetwork/lnd/pull/8813)
  adds support for channel points.

+* [BuildRoute](https://github.com/lightningnetwork/lnd/pull/8886) now supports


Need to move those to the 0.18.3 file now.

guggero · 2024-07-24T12:25:08Z

routing/router.go

+	incoming, outgoing *unifiedEdge) lnwire.MilliSatoshi {
+
+	// TODO: calculations with integer ceil/floor arithmetic
+	feeRateParts := 1e6


I think we can just keep everything multiplied by a million and then do integer division later on?
Not sure about the requirements of the actual algorithm, I just know we should try to avoid float64 wherever possible!
But I guess that's exactly what the TODOs here are for.

It was kind of tough to do integer arithmetic here because formulas involve products of PPM * PPM * amount, which can easily overflow a int64 and we need to handle negative numbers as well. I tried an approach with floats, they at least don't have a problem overflowing when adding/multiplying numbers and I think there are no problems related to accumulating errors, but not too knowledgeable here. Another option would be to use big.Int, but the syntax there looks very clumsy

We split up the functionality in getRouteUnifiers into checking that all edges exist via getEdgeUnifiers and then add a backward pass that will be responsible for determining the sender amount. We remove the node pub key from the error string, as in route building this is duplicate info, which can be determined from the input keys, further it's not available in the backward pass anymore. We refactor the BuildRoute test to use the require library and add a test case for a max HTLC violation on the last hop.

bitromortac · 2024-07-30T17:22:12Z

Thanks for the reviews @guggero and @ProofOfKeags and also for the improvement suggestions!

My understanding is that there are broadly two cases that we need to consider

Yes pretty well described 👍

The algorithm seems to be intrinsically imperative and essentially must be computed iteratively.
From here It comes to my attention that the following expressions should hold

sendAmt := recvAmt + fees
fees := outboundFees + inboundFees
outboundFees := fn.Sum(fn.Map(getOutboundFee, edges[1:]))
inboundFees := fn.Sum(fn.Map(getInboundFee, edges[:len(edges)-1]))

Roughly yes, unfortunately the inbound fees are coupled to the previous hop's outbound fees, which is why I think the map approach wouldn't work well.

I added some documentation to the amount calculation, hope that helps. I'm still using float numerics, but need to look into that again to be sure it is precise and what the boundaries of failures would be.

However, in other parts of LND I've been favoring the approach of "no PR should make the quality of the codebase deteriorate".

Totally agree, happy to address things!

routing/router.go

ProofOfKeags

Much improved. I still have some stuff I'd like to see addressed, particularly around testing and floating point arithmetic.

routing/router.go

ProofOfKeags · 2024-07-30T22:46:49Z

routing/router.go

+	// Ai(Ao) can potentially become more negative in amplitude than Ao,
+	// which is why the above mentioned capping is needed. We can abbreviate
+	// Ai(Ao) with Ai(Ao) = m*Ao + n, where m and n are:
+	// m := (1 + Ro/PPM) * (1 + Ri/PPM)


I believe this expression only holds in integer space, not float space.

not sure I follow, could you explain that?

What I mean is that division has different composability properties when done in integer space vs real space. For instance: A == (A / B) * B is not an invariant that holds in integer space. As such, I believe that the expression (1 + Ro/PPM) * (1 + Ri/PPM) is an expression that is only valid when we are doing integer math. I think that now that you've refactored towards using bigints this should be fine and we can probably ignore my comment above.

Hm, I'm still a bit confused by the statement, since I'd rather say it's accurate in real space (up to a cast to compare to the int result) but not in integer space as you need to do an integer division twice with the accompanied rounding, but good it's resolved

I thought the reason we add the 1 in the first place is because of the truncation of the remainder in integer space. By keeping it in real space I do not believe we need the 1 + part of the expression. Maybe I misunderstand.

It's not about rounding here, it's the exact formula, but factored to see how the fee rates compare to 1, to see if it results in a zero or negative overall term, see the og derivation https://github.com/feelancer21/lnd/blob/f6f05fa930985aac0d27c3f6681aada1b599162a/prototype/doc.md#outgoing---incoming.

routing/router.go

routing/router_test.go

routing/router.go

We duplicate the function calls to handle the min amount and known amount cases in a similar manner, to make the next diff easier to parse.

We shift the duty of determining the policies to the backward pass as the forward pass will only be responsible for finding the corrected receiver amount. Note that this is not a pure refactor as demonstrated in the test, as the forward pass doesn't select new policies anymore, which is less flexible and doesn't lead to the highest possible receiver amount. This is however neccessary as we otherwise won't be able to compute forwarding amounts involving inbound fees and this edge case is unlikely to occur, because we search for a min amount for a route that was most likely constructed for a larger amount.

bitromortac · 2024-08-01T09:33:27Z

I switched outgoingFromIncoming to use big.Int to avoid any overflow and accuracy issues with int64 or float64, even in places where we would probably not need it, but just to be consistent. Also added a commit to switch to use an Option type for the amount. Will fix the linter complaint in the next round.

ProofOfKeags

Super close at this point.

routing/router.go

ProofOfKeags · 2024-08-01T19:24:58Z

routing/router.go

+	// Ai(Ao) can potentially become more negative in amplitude than Ao,
+	// which is why the above mentioned capping is needed. We can abbreviate
+	// Ai(Ao) with Ai(Ao) = m*Ao + n, where m and n are:
+	// m := (1 + Ro/PPM) * (1 + Ri/PPM)


What I mean is that division has different composability properties when done in integer space vs real space. For instance: A == (A / B) * B is not an invariant that holds in integer space. As such, I believe that the expression (1 + Ro/PPM) * (1 + Ri/PPM) is an expression that is only valid when we are doing integer math. I think that now that you've refactored towards using bigints this should be fine and we can probably ignore my comment above.

routing/router.go

routing/router_test.go

routing/router.go

ProofOfKeags · 2024-08-06T20:45:18Z

routing/router.go

-	error) {
+// getEdgeUnifiers returns a list of edge unifiers for the given route.
+func getEdgeUnifiers(source route.Vertex, hops []route.Vertex,
+	outgoingChans map[uint64]struct{},


fwiw this is is a fn.Set[uint64]

I agree, a set would be nice here. But changing that would also require a change in newNodeEdgeUnifier which is used in a couple of places. So non-blocking IMO, don't want to turn this into a big type refactor PR.

routing/router.go

ProofOfKeags · 2024-08-06T21:07:50Z

routing/router.go

+	// Ai(Ao) can potentially become more negative in amplitude than Ao,
+	// which is why the above mentioned capping is needed. We can abbreviate
+	// Ai(Ao) with Ai(Ao) = m*Ao + n, where m and n are:
+	// m := (1 + Ro/PPM) * (1 + Ri/PPM)


I thought the reason we add the 1 in the first place is because of the truncation of the remainder in integer space. By keeping it in real space I do not believe we need the 1 + part of the expression. Maybe I misunderstand.

ProofOfKeags · 2024-08-06T21:12:21Z

routing/router.go

+			receiverAmt, err := receiverAmtForwardPass(
+				senderAmt, pathEdges,
+			)

-		receiverAmt = *amt
+			return fn.NewResult(receiverAmt, err)


I learned from @Roasbeef that you can actually just do this and it mysteriously works:

func() fn.Result[lnwire.MilliSatoshi] { return fn.NewResult( receiverAmtForwardPass( senderAmt, pathEdges, ) ) }

guggero

Looks very nice!
Two questions and a nit, other than that LGTM 🎉

guggero · 2024-08-07T08:01:32Z

routing/router.go

-	error) {
+// getEdgeUnifiers returns a list of edge unifiers for the given route.
+func getEdgeUnifiers(source route.Vertex, hops []route.Vertex,
+	outgoingChans map[uint64]struct{},


I agree, a set would be nice here. But changing that would also require a change in newNodeEdgeUnifier which is used in a couple of places. So non-blocking IMO, don't want to turn this into a big type refactor PR.

guggero · 2024-08-07T08:13:17Z

routing/router.go

-		localChan := i == 0
-		edgeUnifier := unifiers[i]
+	// We traverse the route backwards and handle the last hop separately.
+	edgeUnifier := unifiers[len(unifiers)-1]


Do we need a bounds check here or are we certain there's always at least one edge?

nice, I added a check here and on the RPC level (new commit)

guggero · 2024-08-07T08:26:04Z

routing/router_test.go

+		tt := tt
+
+		t.Run(tt.name, func(t *testing.T) {
+			testInboundOutboundFee(t,


micro-nit: t on new line. Also, usually we use tc for test cases and tt for sub *testing.T within a t.Run(). But really inconsequential, feel free to ignore.

cool, I never realized that pattern, updated 🙏

Adds a utility function to be able to compute the outgoing routing amount from the incoming amount by taking inbound and outbound fees into account. The discussion was contributed by user feelancer21, see feelancer21@f6f05fa.

guggero

LGTM 🎉

bitromortac · 2024-08-07T12:22:55Z

I noticed that when constructing the edge unifiers, we always included inbound fees in the policy. In pathfinding we don't set them to be active for the last hop, which is important for edge constraint checks (updated and documented in TestSenderAmtBackwardPass). I also added a test case for a min amount search with inbound fees that also displays an interesting edge case with rounding, which gets corrected in newRoute. I think that an itest could be interesting here as well, on the otherhand, because everything passes through newRoute, this should be covered by the pathfinding tests.

saubyk added this to the v0.18.2 milestone Jul 4, 2024

feelancer21 mentioned this pull request Jul 7, 2024

Support Inbound Fees when LND 18 is released. rkfg/regolancer#63

Open

saubyk assigned bitromortac Jul 9, 2024

saubyk requested a review from ProofOfKeags July 9, 2024 16:44

saubyk added routing inbound fee Changes related to inbound routing fee P1 MUST be fixed or reviewed labels Jul 15, 2024

ProofOfKeags requested changes Jul 15, 2024

View reviewed changes

saubyk requested a review from guggero July 23, 2024 16:40

guggero reviewed Jul 24, 2024

View reviewed changes

bitromortac added 2 commits July 30, 2024 15:49

routing: reorganize BuildRoute

16f6284

bitromortac force-pushed the buildroute-inbound-fees branch from eb4cf50 to 3b15f4c Compare July 30, 2024 15:59

bitromortac requested review from ProofOfKeags and guggero July 30, 2024 17:22

bitromortac marked this pull request as ready for review July 30, 2024 17:22

bitromortac commented Jul 30, 2024

View reviewed changes

routing/router.go Outdated Show resolved Hide resolved

ProofOfKeags requested changes Jul 30, 2024

View reviewed changes

guggero removed their request for review July 31, 2024 09:03

bitromortac force-pushed the buildroute-inbound-fees branch from 3b15f4c to 1021d3e Compare July 31, 2024 15:35

ProofOfKeags reviewed Jul 31, 2024

View reviewed changes

routing/router.go Show resolved Hide resolved

ProofOfKeags reviewed Jul 31, 2024

View reviewed changes

routing/router.go Outdated Show resolved Hide resolved

ProofOfKeags reviewed Jul 31, 2024

View reviewed changes

routing/router.go Outdated Show resolved Hide resolved

ProofOfKeags reviewed Jul 31, 2024

View reviewed changes

routing/router.go Show resolved Hide resolved

bitromortac added 2 commits August 1, 2024 11:16

routing: handle both amounts equally

e4c7c10

We duplicate the function calls to handle the min amount and known amount cases in a similar manner, to make the next diff easier to parse.

bitromortac force-pushed the buildroute-inbound-fees branch from 1021d3e to 55c590b Compare August 1, 2024 09:22

bitromortac requested review from guggero and ProofOfKeags August 1, 2024 11:00

ProofOfKeags reviewed Aug 1, 2024

View reviewed changes

bitromortac force-pushed the buildroute-inbound-fees branch from 55c590b to 8c779d8 Compare August 5, 2024 16:54

bitromortac requested a review from ProofOfKeags August 5, 2024 19:18

saubyk linked an issue Aug 6, 2024 that may be closed by this pull request

[bug]: routerClient.BuildRoute does not consider inbound fees #8814

Closed

ProofOfKeags approved these changes Aug 6, 2024

View reviewed changes

guggero reviewed Aug 7, 2024

View reviewed changes

bitromortac added 2 commits August 7, 2024 12:40

routing: refactor backward and forward pass

2c79bf9

routing: add outgoingFromIncoming amount calc

36cd036

Adds a utility function to be able to compute the outgoing routing amount from the incoming amount by taking inbound and outbound fees into account. The discussion was contributed by user feelancer21, see feelancer21@f6f05fa.

bitromortac force-pushed the buildroute-inbound-fees branch from 8c779d8 to 65d7515 Compare August 7, 2024 10:53

guggero approved these changes Aug 7, 2024

View reviewed changes

bitromortac force-pushed the buildroute-inbound-fees branch from 65d7515 to e5bbe27 Compare August 7, 2024 12:16

bitromortac added 4 commits August 7, 2024 14:23

routing: add inbound fee support for BuildRoute

8b32e3e

lnrpc+routing: convert amt pointer to fn.Option

452db01

routerrpc: add check for empty hops

b0e1a72

docs: add to release notes

f622b43

bitromortac force-pushed the buildroute-inbound-fees branch from e5bbe27 to f622b43 Compare August 7, 2024 12:26

guggero merged commit b63e5de into lightningnetwork:master Aug 7, 2024
28 of 33 checks passed

feelancer21 mentioned this pull request Aug 7, 2024

router: inbound fees support for BuildRoute #7060

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

routing: inbound fees support for BuildRoute #8886

routing: inbound fees support for BuildRoute #8886

bitromortac commented Jul 2, 2024 •

edited

Loading

coderabbitai bot commented Jul 2, 2024 •

edited

Loading

Review skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

ProofOfKeags left a comment

guggero left a comment

guggero Jul 24, 2024

guggero Jul 24, 2024

bitromortac Jul 30, 2024 •

edited

Loading

bitromortac commented Jul 30, 2024

ProofOfKeags left a comment

ProofOfKeags Jul 30, 2024

bitromortac Jul 31, 2024

ProofOfKeags Aug 1, 2024

bitromortac Aug 5, 2024

ProofOfKeags Aug 6, 2024

bitromortac Aug 7, 2024

bitromortac commented Aug 1, 2024 •

edited

Loading

ProofOfKeags left a comment

ProofOfKeags Aug 1, 2024

ProofOfKeags Aug 6, 2024

guggero Aug 7, 2024

ProofOfKeags Aug 6, 2024

ProofOfKeags Aug 6, 2024

guggero left a comment

guggero Aug 7, 2024

guggero Aug 7, 2024

bitromortac Aug 7, 2024

guggero Aug 7, 2024

bitromortac Aug 7, 2024

guggero left a comment

bitromortac commented Aug 7, 2024 •

edited

Loading

routing: inbound fees support for BuildRoute #8886

routing: inbound fees support for BuildRoute #8886

Conversation

bitromortac commented Jul 2, 2024 • edited Loading

coderabbitai bot commented Jul 2, 2024 • edited Loading

Review skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

ProofOfKeags left a comment

Choose a reason for hiding this comment

guggero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitromortac Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

bitromortac commented Jul 30, 2024

ProofOfKeags 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitromortac commented Aug 1, 2024 • edited Loading

ProofOfKeags 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

Choose a reason for hiding this comment

guggero 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

Choose a reason for hiding this comment

guggero left a comment

Choose a reason for hiding this comment

bitromortac commented Aug 7, 2024 • edited Loading

bitromortac commented Jul 2, 2024 •

edited

Loading

coderabbitai bot commented Jul 2, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

bitromortac Jul 30, 2024 •

edited

Loading

bitromortac commented Aug 1, 2024 •

edited

Loading

bitromortac commented Aug 7, 2024 •

edited

Loading