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

lnwire: hard reject positive inbound fees set over RPC #8605

Open
Roasbeef opened this issue Mar 30, 2024 · 10 comments
Open

lnwire: hard reject positive inbound fees set over RPC #8605

Roasbeef opened this issue Mar 30, 2024 · 10 comments
Assignees
Labels
beginner Issues suitable for new developers enhancement Improvements to existing features / behaviour fees Related to the fees paid for transactions (both LN and funding/commitment transactions) inbound fee Changes related to inbound routing fee P1 MUST be fixed or reviewed rpc Related to the RPC interface validation

Comments

@Roasbeef
Copy link
Member

In #6703, we'll add initial support for inbound fees. Due to deployment considerations, only inbound fees should be used, as they're backwards compatible. An unupgraded sender will end up missing out on the discount, but the payment will go through if negative fees are used. If positive fees are used, then the forwarding node may reject a payment from an unupgraded sender. Additionally, in order for the sender to be able to properly retry, they'll also need the inbound fee policy in addition to the outbound fee policy. lnd isn't yet able to send both policies until #6967 is merged.

To ensure the base feature minimized disruption in the network, we should also modify the RPC code to reject positive inbound fees. The CLI code will already do this.

@Roasbeef Roasbeef added enhancement Improvements to existing features / behaviour rpc Related to the RPC interface beginner Issues suitable for new developers fees Related to the fees paid for transactions (both LN and funding/commitment transactions) validation inbound fee Changes related to inbound routing fee labels Mar 30, 2024
@saubyk saubyk added the P1 MUST be fixed or reviewed label Apr 2, 2024
@joostjager
Copy link
Contributor

joostjager commented Apr 4, 2024

The CLI update should indeed prevent an ad-hoc unintentional positive inbound fee from being set.

But for applications that are using the API and to which inbound fee support is added specifically, this safe guard is perhaps a bit too much hand-holding? The down side of the API restriction is that even if someone would want to try experimenting with positive inbound fees (perhaps they are confident that enough senders have upgraded to 0.18), they cannot do it and need to wait for and upgrade to a future version of lnd. Enabling experimentation was the main goal of the inbound fee change, so I'd argue to keep the option open on the API level.

#6967 isn't just relevant for positive inbound fees, but also for negative inbound fees that are made less negative.

@feelancer21
Copy link
Contributor

If external tools have implemented incoming fees, unintentional use can also occur via the API. Imho this is also more likely as many users do their policy settings with tools like lndg or charge-lnd.

I suggest that positive inbound charges are disabled by default, but the user has the option to enable the experimental use via the configuration.

@Roasbeef
Copy link
Member Author

If external tools have implemented incoming fees, unintentional use can also occur via the API. Imho this is also more likely as many users do their policy settings with tools like lndg or charge-lnd.

Yeah this captures my fear: users see the value, set it to somethign positive, then wonder why all forwards are being/failed rejected.

@joostjager
Copy link
Contributor

joostjager commented Apr 15, 2024

I just meant to say that someone developing a 3rd party tool and using the api is probably more aware of the consequences of using positive inbound fees and will make a sensible decision how to offer the functionality to the tool's users. Where as with a casual cli user, the risk of making an unintentional change might be higher. Although setting a positive fee and seeing traffic go away isn't the end of the world either.

How about adding a bool flag to the api call allow_positive_inbound_fee, surrounded with warningful docs?

@feelancer21
Copy link
Contributor

feelancer21 commented Apr 15, 2024

@joostjager I have already started Pull Request #8627. I think it is better to have an activation flag for the positive fees in lnd.conf. Then it is disabled per default and the users who want to play around with this feature can enable it.

@joostjager
Copy link
Contributor

Ah didn't notice that one. Why do you think it is better?

@ziggie1984
Copy link
Collaborator

ziggie1984 commented Apr 15, 2024

Maybe the flag on the API level is more flexible and people don't need to restart LND. Tho regarding LND's history it feels like adding a config flag is more in line with the previous design choices, for example accept-keysend, protocol.wumbo-channels etc., but I am open to both approaches.

@feelancer21
Copy link
Contributor

The reason is that we also have to think about the users of 3rd party tools. It would be quite easy for a developer of the tool to implement a logic that sets a boolean flag to true if the inbound fee is greater than 0. But then it is up to the user to set the right sign when configuring the tool. I believe that many users of a 3rd party tool would confuse fee and discount at the beginning and unintentionally set a positive one, although they actually only want a discount.
In my opinion, the config option represents an additional hurdle and the warnings in the sample will hopefully motivate the user to look more closely at the topic.

@joostjager
Copy link
Contributor

joostjager commented Apr 15, 2024

Tho regarding LND's history it feels like adding a config flag is more in line with the previous design choices, for example accept-keysend, protocol.wumbo-channels etc., but I am open to both approaches.

One difference with those settings is that they also control logic that isn't directly activated via an rpc call (receive payment, accept channel).

I prefer to remain maximally flexible and even think the restriction is not necessary at all. But if you want to have the restriction, I am already happy when it isn't hard-coded and implemented as a config file setting instead.

@kaloudis
Copy link
Contributor

The reason is that we also have to think about the users of 3rd party tools. It would be quite easy for a developer of the tool to implement a logic that sets a boolean flag to true if the inbound fee is greater than 0. But then it is up to the user to set the right sign when configuring the tool. I believe that many users of a 3rd party tool would confuse fee and discount at the beginning and unintentionally set a positive one, although they actually only want a discount. In my opinion, the config option represents an additional hurdle and the warnings in the sample will hopefully motivate the user to look more closely at the topic.

this is a great point. I think this either has to be an opt-in config setting, or perhaps even a separate API tucked under devrpc until the functionality is widespread across the network.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginner Issues suitable for new developers enhancement Improvements to existing features / behaviour fees Related to the fees paid for transactions (both LN and funding/commitment transactions) inbound fee Changes related to inbound routing fee P1 MUST be fixed or reviewed rpc Related to the RPC interface validation
Projects
None yet
Development

No branches or pull requests

6 participants