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

fundingmanager: configurable remote max htlcs #4527

Merged

Conversation

cfromknecht
Copy link
Contributor

This PR adds the ability to configure the remote max-htlcs value during
channel funding in two ways:

  • a new configuration option, default-remote-max-htlcs, which sets the
    default value for both the initiator and acceptor of the funding flow.
  • a new openchannel option, remote-max-htlcs, allowing the initiator to
    specify the acceptor's max-htlcs value, overriding default-remote-max-htlcs
    for a specific channel.

The motivation for doing so is to allow node operators and wallets to limit exposure
to the maximum number of concurrent HTLCs that can be present on the commitment
at a given time. Most nodes will never truly need the capacity specified by the protocol
maximum of 483 HTLCs (in each direction). However, recent works have also shown that
lower values here can meaningfully reduce the exposure to chain fees and network
congestion during unilateral closures. As such, this provides an initial set of controls
to migrate away from using the protocol maximum by default.

NOTE: this PR does not support specifying an override value during channel acceptance.
Down the road I think it makes sense to expose this as part of the channel_acceptor RPC.
In addition to simply accepting/rejecting an incoming channel, users would be able to
specify any of the parameters passed to openchannel as part of the response. This refactor
is quite a bit larger, so we've started with the two controls listed above.

Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

LGTM ⚡

@joostjager
Copy link
Contributor

How does this approach compare to limiting the number of htlcs via the htlc interceptor (may need some extension)? Htlc interceptor provides more flexibility, but I guess it doesn't allow limiting the number of incoming htlcs.

However, recent works have also shown that lower values here can meaningfully reduce the exposure to chain fees and network congestion during unilateral closures.

Do you have a link?

@cfromknecht
Copy link
Contributor Author

How does this approach compare to limiting the number of htlcs via the htlc interceptor (may need some extension)? Htlc interceptor provides more flexibility, but I guess it doesn't allow limiting the number of incoming htlcs.

This doesn't actually limit your exposure because the remote party can still add up to 483 HTLCs which you can't reject until after they've been added to a signed commitment. It is also more difficult to set up and operate than a config option. Users/developers that don't take the time to set this up would find themselves, knowingly or unknowingly, creating/accepting channels with the protocol maximum.

Do you have a link?

Two of the more recent works:

  1. https://arxiv.org/pdf/2006.08513.pdf
  2. https://lists.linuxfoundation.org/pipermail/lightning-dev/2020-June/002735.html

To be fair, this does not completely resolve either issue. But it does lessen the worst-case chain space/channel in [1] and total fees paid at a given fee rate in [2]. For example, a wide-spread uptake of max-htlcs=50 (as is done in this PR) would translate to a roughly 10x increases in the number of channel closures required to reliably pull off [1].

@joostjager
Copy link
Contributor

This doesn't actually limit your exposure because the remote party can still add up to 483 HTLCs which you can't reject until after they've been added to a signed commitment. It is also more difficult to set up and operate than a config option. Users/developers that don't take the time to set this up would find themselves, knowingly or unknowingly, creating/accepting channels with the protocol maximum.

It is also possible to implement this more dynamic limitation in lnd where it can still be a config option. The immutable channel property would then still be maximally flexible. But indeed, for the incoming htlcs they'd need to be added then cancelled. That does prevent [1], because the htlcs aren't locked in? And I think [2] is prevented as well, because it needs a complete path to the attacker destination node?

@joostjager
Copy link
Contributor

Discussed offline: dynamic limitation doesn't work because you can't cancel back a locked-in htlc without cooperation from the other party.

@Roasbeef Roasbeef added config Parameters/arguments/config file related issues/PRs funding Related to the opening of new channels with funding transactions on the blockchain v0.12 labels Aug 21, 2020
@Roasbeef Roasbeef modified the milestones: 0.11.1, 0.12.0 Aug 21, 2020
@Crypt-iQ Crypt-iQ self-requested a review August 21, 2020 16:46
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 🥶

@Roasbeef
Copy link
Member

Needs a rebase!

Otherwise the fatal error message incorrectly describes what happpened.
Currenlty the maxHtlcs value is recomputed after receiving
accept_channel. This works when the derivation is deterministic, howver
we now allow the user to manually override this value from open_channel.
As such, we must retain the chosen value in memory throughout the
funding process, otherwise the initiator would revert to the
deterministic derivation and the two endpoints will disagree on the
correct max-htlcs value in their view of the other's policy.
This will allow users to set a custom limit for the max number of
concurrent HTLCs the remote party can add to the channel.
@cfromknecht
Copy link
Contributor Author

@Roasbeef rebased. default is changed to 483 so that we don't have any abrupt changes in 0.11.1, with the intention of lowering to something like 50 for 0.12

@Crypt-iQ
Copy link
Collaborator

Looks good to me ✉️

Copy link
Collaborator

@Crypt-iQ Crypt-iQ left a comment

Choose a reason for hiding this comment

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

lgtm

@Roasbeef Roasbeef merged commit 63bd8e7 into lightningnetwork:master Aug 25, 2020
@cfromknecht cfromknecht deleted the configurable-remote-max-htlcs branch August 26, 2020 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config Parameters/arguments/config file related issues/PRs funding Related to the opening of new channels with funding transactions on the blockchain v0.12
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants