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.

Sign up for GitHub

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

Jump to bottom

[anchors] zero-fee HTLC secondlevel transactions #4840

Merged
merged 7 commits into from
Dec 15, 2020
Merged

[anchors] zero-fee HTLC secondlevel transactions #4840

merged 7 commits into from
Dec 15, 2020

Conversation

halseth
Copy link
Contributor

@halseth halseth commented Dec 7, 2020 •
edited
Loading

This PR enables an extension to the anchor commitment type that makes second-level HTLC transactions pay a zero fee, meaning they will have to have fees added as in #4779.

This PR adds this type to the already existing anchors type, meaning all new anchor channels being opened will have this enabled (as long as both nodes support it). Existing anchor channels will stay the same.

TODO

  • Reserve feature bit at spec level

Builds on

#4779

@Roasbeef Roasbeef added this to the 0.12.0 milestone Dec 9, 2020
Roasbeef
Roasbeef reviewed Dec 9, 2020
View reviewed changes
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.

Very straight forward change! No major comments, I think if we're in agreement re 0 fees vs 1 sat/byte, then this can go in as soon as the dependent PRs are merged.

feature/default_sets.go Outdated Show resolved Hide resolved
lnwallet/commitment.go
// For zero-fee HTLC channels, this will always be zero, regardless of
// feerate.
if chanType.ZeroFeeSecondLevel() {
return 0
Copy link
Member

Choose a reason for hiding this comment

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

EZ 😎

Could also instead have the param be passed in (0 in this case) if we want to make it a constant so it can be modified easily later. Based on recent discussions though, it seems like the zero fee route may be the best way forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer zero, since that mitigates the fee leak completely.

chanrestore.go Show resolved Hide resolved
@halseth halseth mentioned this pull request Dec 9, 2020
Merged
@halseth halseth force-pushed the anchors-zero-fee-secondlevel branch 2 times, most recently from 8f6be06 to f6404f6 Compare December 9, 2020 12:08
halseth
halseth commented Dec 9, 2020
View reviewed changes
chanbackup/single.go Outdated

// ZeroFeeSecondLevelCommitVersion is a version that denotes this
// channel isusing the zero-fee second-level anchor commitment format.
ZeroFeeSecondLevelCommitVersion = 3
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't strictly need a new version, since on recovery we don't handle HTLCs anyway.

@halseth
Copy link
Contributor Author

halseth commented Dec 10, 2020

Updated with latest terminology used in the spec PR: lightning/bolts#824

@halseth halseth mentioned this pull request Dec 10, 2020
Closed
cfromknecht
cfromknecht reviewed Dec 12, 2020
View reviewed changes
lnwallet/reservation.go Outdated Show resolved Hide resolved
feature/deps.go Outdated Show resolved Hide resolved
fundingmanager.go Outdated Show resolved Hide resolved
@Roasbeef
Copy link
Member

Needs a rebase!

@halseth halseth force-pushed the anchors-zero-fee-secondlevel branch 3 times, most recently from b81b862 to 387adc6 Compare December 14, 2020 20:07
Roasbeef
Roasbeef reviewed Dec 14, 2020
View reviewed changes
feature/deps.go Outdated
@@ -58,6 +58,7 @@ var deps = depDesc{
lnwire.AnchorsOptional: {
lnwire.StaticRemoteKeyOptional: {},
},
lnwire.AnchorsZeroFeeHtlcTxOptional: {},
Copy link
Member

Choose a reason for hiding this comment

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

Remove optional signal as well? Can defer perhaps until the rc phase as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just the dependency check, I don't think it affects the signalling, @cfromknecht ?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah this can be removed, this is equivalent to not having an entry in the dep map

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, removed! Should we remove the lnwire.AnchorsOptional entry also?

feature/default_sets.go Show resolved Hide resolved
Roasbeef
Roasbeef approved these changes Dec 14, 2020
View reviewed changes
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 🐉

cfromknecht
cfromknecht reviewed Dec 14, 2020
View reviewed changes
feature/deps.go Outdated
@@ -58,6 +58,7 @@ var deps = depDesc{
lnwire.AnchorsOptional: {
lnwire.StaticRemoteKeyOptional: {},
},
lnwire.AnchorsZeroFeeHtlcTxOptional: {},
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah this can be removed, this is equivalent to not having an entry in the dep map

fundingmanager.go
// implicitly mean we'll create the channel of this type. Note that
// this also enables tweakless commitments, as anchor commitments are
// always tweakless.
localAnchors := localFeatures.HasFeature(
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

lncfg/protocol.go

// 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
Contributor

Choose a reason for hiding this comment

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

not sure how specific we want to be here, but does it make sense to clarify that this is anchor_zero_fee_htlc vs just anchor_outputs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think users interacting with this flag don't have to know about the difference.

lncfg/protocol_rpctest.go

// Anchors enables anchor commitments.
// TODO(halseth): transition itests to anchors instead!
Anchors bool `long:"anchors" description:"enable support for anchor commitments"`
Copy link
Contributor

Choose a reason for hiding this comment

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

before this was NoAnchors, now it's back to Anchors?

Copy link
Member

Choose a reason for hiding this comment

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

So there's two different versions of the config now: one for the itests, and the one for the rest. This commit makes the default one have anchor on, while the itest one it's off by default.

@Roasbeef Roasbeef merged commit d289a6f into lightningnetwork:master Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Roasbeef Roasbeef Roasbeef approved these changes

@cfromknecht cfromknecht cfromknecht left review comments

Assignees
No one assigned
Labels
v0.12
Projects
None yet
Milestone
0.12.0
Development

Successfully merging this pull request may close these issues.
3 participants
@halseth @Roasbeef @cfromknecht