lnd
lnd copied to clipboard
lightningnetwork
routing: inbound fees send support
Implements send support for inbound routing fees. The receiver side is implemented in #6703 and this PR is based off of that.
Implementation details
- The
channel_updateextra opaque data isn't validated before it was stored in the database. This means that everywhere we read this data, a tlv error might be encountered. In this PR, this case is handled in a mostly defensive way, skipping over the channel policy.
Based on #6703. Review first commits there.
Support for inbound fees in BuildRoute is explored in #7060.
Removed exit hop inbound fees. Previous code saved in https://github.com/lightningnetwork/lnd/compare/master...bottlepay:lnd:inbound-fees-send-support-exit-hop
What would be needed to get his PR out of draft?
I think it is pretty complete as is. Definitely good enough to be moved out of draft, so I did.
Would you keep the current penalization scheme for failures? It seems like with inbound discounts, the channel partner would also need to be blamed more, as they can offset outbound fees of their peer, where a payment attempt wouldn't have happened otherwise with high outbound fees only.
Afaik when the node with a high inbound discount is not delivering, it is penalized as part of a pair of nodes. Do you think that is insufficient?
I think it would be safer for people to experiment with if negative fees were scoped out so that no net negative possibility was allowed - lower bound on a fee would be zero basically, this kind of thing could also be added in the proto comments or somewhere to tell people how to play with it
One thing to keep in mind is that there are two definitions of the 'total fee':
- Node total fee: the sum of inbound and outbound fee charged by a routing node.
- Channel total fee: the outbound fee charged at the sending end of a channel plus the inbound fee charged at the receiving end of that same channel.
For example in the example below, channel total fee is -2 (negative) but node total fee (for B) is 1 (positive).
[A] out: 5 ---> in: -7 [B] out: 8 --->
In #6703, a protection is implemented so that nodes only forward when node total fee is non-negative.
Pathfinding however is a channel-based search. Each round the route is extended with another channel based on the channel total fee. Negative channel total fees are already treated as being zero, to prevent nodes from making themselves infinitely attractive by means of a deeply negative fee. Preventing node total fee from being negative might be complicated because when a channel is selected, it isn't clear yet what the next (or actually previous, because of backwards search) is going to be.
When a best route has been found however, a second pass is going to redo the fees and take advantage of any negative fees that might have been rounded up to zero during search. @bitromortac suggested to not do that. Note that this is the other total fee again, node total fee.
Afaik when the node with a high inbound discount is not delivering, it is penalized as part of a pair of nodes. Do you think that is insufficient?
Right, but it wouldn't have an effect on the inbound fee node's reputation as only the outgoing pairs count for that (there would be only incoming info in the scenario here), if I understand correctly. An alternative would be to also fail the reverse direction should (outbound fee + inbound fee) < threshold, in case of a temporary channel failure. The threshold could be dependent on the outbound fee, perhaps half of it?
One thing to keep in mind is that there are two definitions of the 'total fee'
Great point, thanks for the clarification, I was aware that there are two ways to compute a fee, but not 100% aware that the pathfinder uses the total channel fee as opposed to route construction where the node total fee is used.
I think it would be safer for people to experiment with if negative fees were scoped out so that no net negative possibility was allowed - lower bound on a fee would be zero basically, this kind of thing could also be added in the proto comments or somewhere to tell people how to play with it
Just wanted to recap, so it makes sense to limit the node total fees to zero as a safety measure on the forwarding side, which is already the case in the other PR. On the sender side here, only the total channel fees are limited to zero, because a way to deal with negative fees is not obvious (and nodes shouldn't be able to make them too attractive).
This can lead to routes that exhibit negative node total fees if the inbound discounts are not capped as mentioned, which leads to routing failures. The sender doesn't know whether the router allows negative fees, so the sender would be more successful if it always constructs routes with non-negative total node fees. On the other hand, a sender can upgrade any time (and there are different implementations) and once they figure out how they want to exploit negative fees they could opt into that if routers allow for it. This looks like routers would need to signal that they allow negative fees, which then could be used in route construction to decide on whether node total fees should be capped or not?
Afaik when the node with a high inbound discount is not delivering, it is penalized as part of a pair of nodes. Do you think that is insufficient?
Right, but it wouldn't have an effect on the inbound fee node's reputation as only the outgoing pairs count for that (there would be only incoming info in the scenario here), if I understand correctly. An alternative would be to also fail the reverse direction should (outbound fee + inbound fee) < threshold, in case of a temporary channel failure. The threshold could be dependent on the outbound fee, perhaps half of it?
In the scenario that we are talking about, the node with the high inbound discount is malicious. But a malicious node is - also without inbound fees - able to direct a penalty to either its incoming or outgoing side depending on the type of failure that it returns.
I don't fully understand what you mean by reverse direction and how a threshold comes into play. I think there is a case to be made for stronger penalization, but that this case is pre-existing. In fact, when building the current penalization scheme, there also was a variant with undirected pair penalties.
Just wanted to recap, so it makes sense to limit the node total fees to zero as a safety measure on the forwarding side, which is already the case in the other PR. On the sender side here, only the total channel fees are limited to zero, because a way to deal with negative fees is not obvious (and nodes shouldn't be able to make them too attractive).
This can lead to routes that exhibit negative node total fees if the inbound discounts are not capped as mentioned, which leads to routing failures. The sender doesn't know whether the router allows negative fees, so the sender would be more successful if it always constructs routes with non-negative total node fees. On the other hand, a sender can upgrade any time (and there are different implementations) and once they figure out how they want to exploit negative fees they could opt into that if routers allow for it. This looks like routers would need to signal that they allow negative fees, which then could be used in route construction to decide on whether node total fees should be capped or not?
I think it is worth going back to the starting point with this. The cap in the other PR was added to prevent routing node operators from accidentally losing money on forwards because of misconfiguration. It is implemented as part of the forwarding logic, but it could also have been part of the policy update logic. Or even just been enforced externally by node mgmt tools.
In the context of this PR, we are discussing adding even more protection for node operators so that in the case that they made a configuration error, they will not only not lose money, but also that senders are adapting to their configuration error by increasing the fee. With this, there is no reason for routing nodes to worry about those config errors anymore. And indeed, if a routing node operator does want a truly negative fee, they'd need to signal.
Zooming out, it feels that all of this is going into the direction of a complex solution for a simple problem. If you are concerned about the footgun, I think it is better to go back to #6703 and fix the update policy logic so that there can never be a combination of in and out channel that makes for a negative total fee. Something like https://github.com/lightningnetwork/lnd/commit/43cfd243cd6b7079dc2f1c9ae2088350911cda55
In the scenario that we are talking about, the node with the high inbound discount is malicious. But a malicious node is - also without inbound fees - able to direct a penalty to either its incoming or outgoing side depending on the type of failure that it returns.
I don't fully understand what you mean by reverse direction and how a threshold comes into play. I think there is a case to be made for stronger penalization, but that this case is pre-existing. In fact, when building the current penalization scheme, there also was a variant with undirected pair penalties.
Sorry, I should have been clearer, I was thinking about scenario https://github.com/lightningnetwork/lnd/pull/6703#issuecomment-1797918626. The channel A-B is depleted, A charges high fees to disable the channel, B discounts the channel for whatever reason and this leads the total channel fee to be small. Without inbound fees, the channel would not see any traffic, but with the inbound fees, B is able to attract traffic. The failing pair is attributed to the node pair A->B (because A fails back due to missing liquidity), which influences A's node reputation because it's outgoing from A, but not B's node reputation (if I understand the code correctly). I wonder if we could additionally attribute a failure to B->A in order to worsen B's reputation as well, because it was the culprit of the failure. The question is when to do this, it should probably depend on the relative sizes of the inbound and outbound fees.
I think it is worth going back to the starting point with this. The cap in the other PR was added to prevent routing node operators from accidentally losing money on forwards because of misconfiguration. It is implemented as part of the forwarding logic, but it could also have been part of the policy update logic. Or even just been enforced externally by node mgmt tools.
So there are two options, the first is that the forwarder doesn't allow forwarding of negative fees. If they want to support negative fees in the future, they would need to signal it via a node feature bit such that senders can exploit it. Without this signaling it makes sense to disable negative fee route construction here, because otherwise there is a possibility that routes get rejected (the motivation was not to protect routing nodes). The second option is to enable negative fees from the start, but it needs the crosschecks you mentioned, but this would enable negative fees already.
Zooming out, it feels that all of this is going into the direction of a complex solution for a simple problem. If you are concerned about the footgun, I think it is better to go back to https://github.com/lightningnetwork/lnd/pull/6703 and fix the update policy logic so that there can never be a combination of in and out channel that makes for a negative total fee. Something like https://github.com/lightningnetwork/lnd/commit/43cfd243cd6b7079dc2f1c9ae2088350911cda55
Not sure which way is best. Default channel open fees would also need to be taken care of, right? It feels like it is safer to have an explicit check in the switch than relying on the policy state, but it's also great to have negative fees available from the start.
Sorry, I should have been clearer, I was thinking about scenario #6703 (comment). The channel A-B is depleted, A charges high fees to disable the channel, B discounts the channel for whatever reason and this leads the total channel fee to be small. Without inbound fees, the channel would not see any traffic, but with the inbound fees, B is able to attract traffic. The failing pair is attributed to the node pair A->B (because A fails back due to missing liquidity), which influences A's node reputation because it's outgoing from A, but not B's node reputation (if I understand the code correctly). I wonder if we could additionally attribute a failure to B->A in order to worsen B's reputation as well, because it was the culprit of the failure. The question is when to do this, it should probably depend on the relative sizes of the inbound and outbound fees.
Ah, you are talking about node probability specifically, where pair results are extrapolated to a global node probability? Because the pair penalty on its own is probably not a problem as A might not want to do business with B anyway?
Btw, why would A charge high fees to disable a channel instead of actually disabling it? With high fees, there is still the risk of someone using it and penalizing.
Not sure which way is best. Default channel open fees would also need to be taken care of, right? It feels like it is safer to have an explicit check in the switch than relying on the policy state, but it's also great to have negative fees available from the start.
Hmm good point. Those default fees make things complicated again. Let's go for disabling negative fees all the way then indeed. I've tacked on a commit that also updates pathfinding to put a floor on node total fee during search.
With this new addition, maybe your first point is also addressed? The node total fee is now always non-negative in pathfinding, so any kind of mega discount needs to be matched with an equally big outbound fee. That reduces the extent to which a node can make itself attractive to what they can already do today with zero fees?
Ah, you are talking about node probability specifically, where pair results are extrapolated to a global node probability? Because the pair penalty on its own is probably not a problem as A might not want to do business with B anyway?
Right, I was thinking about the node probability, which effectively is used as a kind of reputation or deterrence against misbehavior. Do you mean that there's no disadvantage for A if the B->A pair is failed as well?
Btw, why would A charge high fees to disable a channel instead of actually disabling it? With high fees, there is still the risk of someone using it and penalizing.
I see this rather as a continuous process, because ramping up the fee rate goes in hand with increased traffic and whether there's liquidity left, leftovers can still be used at a higher rate, if there's high demand for it, but statistically the channel should get discouraged. Disabling the channel also is a binary process, so one would have to define a fraction at which a channel gets disabled. Also, disabling is often used as a heuristic to judge if some node signals that their channel partner is down.
With this new addition, maybe your first point is also addressed? The node total fee is now always non-negative in pathfinding, so any kind of mega discount needs to be matched with an equally big outbound fee. That reduces the extent to which a node can make itself attractive to what they can already do today with zero fees?
Route reliability is increased with this :+1:, but I need to check if there are other consequences. Also, do we want to already define a flag for nodes to signal to support negative fees?
Do you mean that there's no disadvantage for A if the B->A pair is failed as well?
No, that wasn't what I meant. Just that penalizing A->B (ignoring the node reputation for now) affects A in that they will have less traffic going to B, but they probably don't care if B is a bad node.
Penalizing B->A will affect A as well for that specific link, and indeed, you could argue that it doesn't matter because B is malicious.
I see this rather as a continuous process, because ramping up the fee rate goes in hand with increased traffic and whether there's liquidity left, leftovers can still be used at a higher rate, if there's high demand for it, but statistically the channel should get discouraged. Disabling the channel also is a binary process, so one would have to define a fraction at which a channel gets disabled. Also, disabling is often used as a heuristic to judge if some node signals that their channel partner is down.
But if there is still liquidity left, why wouldn't A want senders to use it? If B makes it more attractive and more likely for A to earn that high fee, I don't really see the downside. I think there is a speculative component here in how the dynamics will work out exactly. I do not oppose a bidirectional penalty, but I wouldn't want to make a change that affects every existing channel too as part of this PR.
Also, do we want to already define a flag for nodes to signal to support negative fees?
I don't see the need to define the flag if we don't use it yet. And may never do.
Added an evaluatory commit that shows channel-based pathfinding to avoid any recognized or unknown side effects of negative inbound fees.
I am in favour of the non-optimal cutoff solution for now. I think it is sufficiently good to gather feedback on the inbound fees concept, and - as you say - we can revisit node pair search at a later stage. I've archived the relevant commit on a different branch https://github.com/joostjager/lnd/tree/inbound-fees-node-pair-search.
[!IMPORTANT]
Auto Review Skipped
Auto reviews are limited to the following labels: llm-review. Please add one of these labels to enable auto reviews.
Please check the settings in the CodeRabbit UI or the
.coderabbit.yamlfile in this repository.To trigger a single review, invoke the
@coderabbitai reviewcommand.
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?
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
@coderabbitaiin 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
@coderabbitaiin 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.
-
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 pauseto pause the reviews on a PR. -
@coderabbitai resumeto resume the paused reviews. -
@coderabbitai reviewto trigger a review. This is useful when automatic reviews are disabled for the repository. -
@coderabbitai resolveresolve all the CodeRabbit review comments. -
@coderabbitai helpto get help.
Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
CodeRabbit Configration File (.coderabbit.yaml)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yamlfile to the root of your repository. - The JSON schema for the configuration file is available here.
- 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/coderabbit-overrides.v2.json
CodeRabbit Discord Community
Join our Discord Community to get help, request features, and share feedback.
![coderabbitai[bot] avatar](https://avatars.githubusercontent.com/in/347564?v=4 "Published by a pub.dev verified publisher"){kind=small}
@joostjager, remember to re-request review from reviewers when ready
Will be possible to set the default inbound fees on lnd.conf the same as the existing outbound fees? I can't see any modifications related to this new feature and merged PR in the sample-lnd.conf available on the repo, and any related with this: https://github.com/lightningnetwork/lnd/blob/master/sample-lnd.conf#L626-L633
@twofaktor that makes a lot of sense, I've made a tracking issue for your request here: https://github.com/lightningnetwork/lnd/issues/8610. Thanks!