lightningnetwork / lnd

Lightning Network Daemon ⚡️
MIT License
7.71k stars 2.09k forks source link

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

Open Roasbeef opened 7 months ago

Roasbeef commented 7 months ago

In https://github.com/lightningnetwork/lnd/pull/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 https://github.com/lightningnetwork/lnd/pull/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.

joostjager commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

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

ziggie1984 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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.