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

[anchors] cap value reserved for anchor fee bumping #5274

Merged
merged 3 commits into from May 12, 2021

Conversation

halseth
Copy link
Contributor

@halseth halseth commented May 6, 2021

We cap the maximum value we'll reserve for anchor channel fee bumping at
10 times the per-channel amount such that nodes with a high number of
channels don't have to keep around a very large amount for the unlikely
scanario that they all close at the same time.

lnwallet/wallet.go Outdated Show resolved Hide resolved
@Roasbeef Roasbeef added this to the 0.13.0 milestone May 7, 2021
@Roasbeef Roasbeef added this to In progress in v0.13.0-beta via automation May 7, 2021
lnwallet/wallet.go Show resolved Hide resolved
lnwallet/wallet.go Outdated Show resolved Hide resolved
@Roasbeef
Copy link
Member

Roasbeef commented May 7, 2021

Let's also return to default anchors in this PR?

@halseth halseth force-pushed the anchors-reserved-value-max branch 2 times, most recently from e47c105 to 362b204 Compare May 10, 2021 08:16
@halseth
Copy link
Contributor Author

halseth commented May 10, 2021

Let's also return to default anchors in this PR?

Done.

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 🌛

WumboChans bool `long:"wumbo-channels" description:"if set, then lnd will create and accept requests for channels larger chan 0.16 BTC"`

// Anchors enables anchor commitments.
// TODO(halseth): transition itests to anchors instead!
Copy link
Member

Choose a reason for hiding this comment

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

IIRC we had a branch somewhere the (started to?) did this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I had a draft PR for that here: #4931

Closed it since it was only a draft, but can rebase it on top of this and see how it fares.

@Roasbeef Roasbeef moved this from In progress to Reviewer approved in v0.13.0-beta May 10, 2021
@Roasbeef Roasbeef requested a review from yyforyongyu May 11, 2021 00:19
Copy link
Collaborator

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

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

LGTM👍

// currentNumAnchorChans returns the current number of anchor channels the
// wallet should be ready to fee bump if needed.
// currentNumAnchorChans returns the current number of non-private anchor
// channels the wallet should be ready to fee bump if needed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this means from now on, since this will return 0 for private channels, we won't require private channels to reserver anything right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, correct

Anchors bool `long:"anchors" description:"enable support for anchor commitments"`
// NoAnchors should be set if we don't want to support opening or accepting
// channels having the anchor commitment type.
NoAnchors bool `long:"no-anchors" description:"disable support for anchor commitments"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a thought, that it would be nice to not change this field but somehow leave it default to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, IIRC, the flag parser we use cannot handle command line flags being false. So if we set this this true by default it would be no way of opting-out.

We cap the maximum value we'll reserve for anchor channel fee bumping at
10 times the per-channel amount such that nodes with a high number of
channels don't have to keep around a very large amount for the unlikely
scanario that they all close at the same time.
Since private channels (most likely) won't be used for routing other
than as last hop, they bear a smaller risk that we must quickly force
close them in order to resolve a HTLC up/downstream.

In the case where it actually has to be force to resolve a payment (if
it is the first/last hop on a routed payment), we can assume that the
router will have UTXOs available from the reserved value from the
incoming public channel.
This commit again enables anchor channels by default.

During itests we still keep anchors opt-in, as many of the tests rely on
the behaviour of non-anchor channels.
@halseth halseth force-pushed the anchors-reserved-value-max branch from 0b2c7b1 to 820e77d Compare May 11, 2021 08:38
// lnd, or to enable experimental protocol changes.
type ProtocolOptions struct {
// LegacyProtocol is a sub-config that houses all the legacy protocol
// options. These are mostly used for integration tests as most modern
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: extra space

type ProtocolOptions struct {
// LegacyProtocol is a sub-config that houses all the legacy protocol
// options. These are mostly used for integration tests as most modern
// nodes shuld always run with them on by default.
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: shuld -> should

v0.13.0-beta automation moved this from Reviewer approved to Review in progress May 11, 2021
@Roasbeef Roasbeef moved this from Review in progress to Reviewer approved in v0.13.0-beta May 11, 2021
@Roasbeef Roasbeef merged commit 6a2fb31 into lightningnetwork:master May 12, 2021
v0.13.0-beta automation moved this from Reviewer approved to Done May 12, 2021
for _, c := range chans {
cntChannel := func(c *channeldb.OpenChannel) {
// We skip private channels, as we assume they won't be used
// for routing.
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this assumption is incorrect because of non-strict forwarding. If one has a public channel and a private one with the same node, then it's possible to forward through both.

Disclaimer: I really don't understand anchor channels properly yet, so I may be wrong or this is non-issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants