Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

routing: inbound fees send support #6934

Merged
merged 4 commits into from
Mar 31, 2024

Conversation

joostjager
Copy link
Contributor

@joostjager joostjager commented Sep 21, 2022

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_update extra 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.

@joostjager
Copy link
Contributor Author

Removed exit hop inbound fees. Previous code saved in master...bottlepay:lnd:inbound-fees-send-support-exit-hop

@joostjager joostjager force-pushed the inbound-fees-send-support branch 3 times, most recently from fc549fa to f9894dc Compare October 25, 2022 10:58
@saubyk saubyk added the inbound fee Changes related to inbound routing fee label Nov 9, 2022
@saubyk saubyk added this to the v0.17.0 milestone Nov 9, 2022
signedFee := inboundFee + outboundFee
fee := lnwire.MilliSatoshi(0)
if signedFee > 0 {
fee = lnwire.MilliSatoshi(signedFee)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@halseth you expressed concerns about negative fees being able to cancel out probability penalties for unreliable nodes. I've addressed the issue here by always rounding the fee that is used for the edge weight up to zero.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this check must be removed now that the node total fee is guarded against negative. Otherwise pathfinding may return suboptimal routes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am actually quite sure of this. Suppose a node has an in fee of -100000 and an out fee of 100000. Those cancel out perfectly and shouldn't lead to pathfinding that is any different from a zero fee?

Copy link
Contributor Author

@joostjager joostjager Nov 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But... we still need to do this because dijkstra can't handle negative weights 🤷

It is a possibility though to floor at zero the actual edge weight calculated below rather than the fee.

Copy link
Collaborator

@bitromortac bitromortac Nov 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am actually quite sure of this. Suppose a node has an in fee of -100000 and an out fee of 100000. Those cancel out perfectly and shouldn't lead to pathfinding that is any different from a zero fee?

Do you mean that those fees are on a single channel or on a node with incoming and outgoing channels?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean on a node with incoming and outgoing channels. The fee paid to that particular node would be 0, so you'd expect it to be treated no different from a node that actually charges zero fee.

Copy link
Collaborator

@bitromortac bitromortac Dec 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could still be a problem, looking into that. edit: left a main comment #6934 (review)

@joostjager
Copy link
Contributor Author

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.

Copy link
Contributor

coderabbitai bot commented Jan 24, 2024

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.yaml file in this repository.

To trigger a single review, invoke the @coderabbitai review command.

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.

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 a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to 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.yaml file 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.

@joostjager joostjager force-pushed the inbound-fees-send-support branch 3 times, most recently from 96903da to ca5dd90 Compare January 24, 2024 11:27
Copy link
Collaborator

@bitromortac bitromortac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉 (tested pathfinding in a small network as well).

@joostjager
Copy link
Contributor Author

Rebased

@lightninglabs-deploy
Copy link

@joostjager, remember to re-request review from reviewers when ready

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🪞

Considering if we should actually also have a hard reject on the RPC level if users try to set a positive inbound fee. In the future we could lift it given sufficient uptake within the core routing network. We can also add more telemetry using the HTLC interceptor (I think we're missing the field there actually?), which'll allow node operators to log the % of incoming payments that are able to observe their negative inbound fees.


fee = int64(outboundFee) + inboundFee
if fee < 0 {
fee = 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should log these occurrences on the Tracef log level?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure. The zero-floor has now become a feature of inbound fees both on the receiver and sender side. Nodes might use that and it doesn't necessary indicate a configuration mistake - if that was the reason you had in mind for logging?

@Roasbeef
Copy link
Member

Needs rebase + conflicts addressed.

Preparation so that we can have the inbound fee available
in addition to the outgoing policy.
Add sender-side support for inbound fees in pathfinding
and route building.
@Roasbeef Roasbeef merged commit 5599b3c into lightningnetwork:master Mar 31, 2024
26 of 27 checks passed
@twofaktor
Copy link
Contributor

twofaktor commented Mar 31, 2024

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

@Roasbeef
Copy link
Member

Roasbeef commented Apr 1, 2024

@twofaktor that makes a lot of sense, I've made a tracking issue for your request here: #8610. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inbound fee Changes related to inbound routing fee P1 MUST be fixed or reviewed path finding routing
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants