-
Notifications
You must be signed in to change notification settings - Fork 267
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
Adapt max HTLC amount to balance #2703
Conversation
I learned today that we are the only implementation that emits a When we're low on liquidity, we should advertise that by emitting a new
I'm not sure yet which of these options is best. We also discussed adding a new failure message to the spec, to be used when we cannot relay a payment because we lack liquidity. |
220da6f
to
c61d32a
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #2703 +/- ##
==========================================
+ Coverage 85.83% 85.86% +0.03%
==========================================
Files 216 216
Lines 18077 18102 +25
Branches 750 762 +12
==========================================
+ Hits 15516 15543 +27
+ Misses 2561 2559 -2
|
3002dd9
to
79b9ab7
Compare
I chose the option of using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good, the code is nice and concise, but this has a lot of subtle side-effects! I'm curious to see how it behaves in practice.
eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/Channel.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/integration/PaymentIntegrationSpec.scala
Outdated
Show resolved
Hide resolved
Some channels have only a few sats available to send but other nodes don't know it so they try to use them and fail. When the balance goes below configurable thresholds we now advertize a lower maxHtlcAmount. This should reduce the number of failed attempts and benefit the network.
79b9ab7
to
4404273
Compare
I wanted to create a plugin that could change htlc_max and htlc_min in various ways based on channel capacity, liquidity, fees, etc and also that could enable/disable channels at will. But it turned out that there's absolutely no way to programmatically issue an arbitrary channel update. One can only update the routing fees...
Yeah, that's why I wanted a plugin(s) for that. |
This is quite dangerous: for example you mention "enable/disable channels at will", but that's not something that should be exposed to the user, because it must only be done when the peer is offline. There are a lot of ways users would shoot themselves in the foot if we expose too many hooks here, that's why we haven't done so. Also, if we wanted to allow plugins to do that, we would need to defer almost everything channel-update related to plugins, because otherwise eclair's internals would most likely end up undoing what the plugin wants to achieve (and the plugin would undo what eclair's internals do). We'd need to think very carefully about which fields we allow plugins to override and how to handle rate-limiting. It could be worth investigating at some point, but not a high priority right now. |
This correctly reduces the pending changes and takes HTLC fees into account. This change highlights that we cannot simply compare to the previous commitment, because it will also have taken pending changes into account, so we instead compare to the current `channel_update`, which is actually what we want.
eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/channel/states/ChannelStateTestsHelperMethods.scala
Outdated
Show resolved
Hide resolved
I don't find it dangerous to enable/disable channels at will. What I do find dangerous though is the ability to set the base fee to 0 and inability to adjust htlc-min accordingly to avoid free routing. IMHO it's not necessary to be that protective because the users know better want kind of plugins their want to install (or not) on their nodes. |
But that's exactly why we want to hide it from users, you don't know the side-effects that it actually has! Even we thought this would only disable the channel in one direction, while all other implementations will ignore it in both directions when receiving such a
The example above is a very good example of the opposite. Users often want something, but they don't realize the unwanted side effects this will have (and plugin writers generally don't realize it either). I believe it's our job to avoid exposing hooks like those when there's too much of a risk that they'll be misused and end up bringing more drawbacks than benefits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
For example, I don’t think that an average user of LND is way more knowledgeable that an average user of Eclair. Just check the PlebNet Telegram groups out, and you’ll see that by yourself. Yet... $ ./lncli help updatechanstatus
NAME:
lncli updatechanstatus - Set the status of an existing channel on the network.
USAGE:
lncli updatechanstatus [command options] funding_txid [output_index] action
CATEGORY:
Channels
DESCRIPTION:
Set the status of an existing channel on the network. The actions can
be "enable", "disable", or "auto". If the action changes the status, a
message will be broadcast over the network.
…
You can do this kind of stuff from the CLI not even from a plugin! Why so?
That’s a very good example, indeed. Everybody including the PlebNet knew about the issue of 0 base fees and htlc-min for a long time. Unfortunately, Eclair doesn’t provide any help to deal with it… Seriously, I don’t really understand where this premise that you guys have an obligation to nanny the users comes from. What is the target audience of Eclair? The users of custodial wallets or the builder of the custodial wallets? I believe it’s the latter (say “hello” from me to @DerEwige :) ). They know what they need and what kind of unwanted side effect they can have very well. |
I would really like the ability to have more control over my channels. I can see the potential danger for the cli, but think a “safety switch” in the config could help with that. But I don’t really see an issue for plugins. I currently do have several use cases, where I would need the ability to have better control. 1.) LND still not able to do spec compliant mutual close To prevent FCs when closing channels to LND I implemented a save close feature. The option to disable the channel would make that much saver/easier. 2.) I monitor the health of my peers (ping response time, HTLC processing time, number of stuck HTLC on the channel, …). The option to disable the channel would make that much saver/easier. 3.) I use dynamic max_htlc for a year now to help with traffic flow and minimize local failures. 4.) Because I use zero base fee, I have adaptive min_htlc that make sure any forward will always pay 1 milli Satoshi in fee (freeloader protection) Again the same issue as with max_htlc. I would really love to have full control over at least those 3 channel stats. |
I've never said the opposite? But exposing all those hooks is one of the reason the main instabilities of the network come from LND nodes, because it's often a faulty plugin that creates incompatibilities and degrade payment reliability and compatibility.
I'm not sure what you're thinking of here? Which issue about 0 base fees? This isn't what I was referring to.
Lightning is extremely complex: we can't expect people to understand the side effects of some of the things they want to achieve. And lightning isn't bitcoin: running a faulty node actually harms the network and real users. Our goal is to give the right set of knobs to ensure people can build useful tools, while protecting them from shooting themselves in the foot. If you think this isn't our job, then we disagree, and that's ok! |
It shouldn't be our responsibility to maintain additional code to work around other implementations bugs. We're not working on short-term timelines here: those bugs will be fixed in LND which will resolve this issue.
But you simply can't: as explained above, using the
That's exactly what I mentioned earlier: giving plugins the ability to manage |
Some channels have only a few sats available to send but are not currently disabled, leading other peers to try to use them and fail. We now
disable channelslower the maximum HTLC amount when the balance goes below a configurable threshold. This should reduce the number of failed attempts and benefit the network.