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

[anchor] pluggable anchor commitments #3821

Merged
merged 31 commits into from Mar 10, 2020

Conversation

@halseth
Copy link
Collaborator

halseth commented Dec 11, 2019

Missing from end-to-end anchors working:

  • Commitment resolution for delayed to_remote output
  • Negotiating anchor type during funding flow

TODO:

  • Unit tests for anchor specific changes
  • One itest case using anchors: #4001

Long term:

  • Automatically sweep anchors
  • whole itest package run using anchors
  • watchtower support
  • channel backups

Builds on #3829

input/script_utils.go Outdated Show resolved Hide resolved
theirBalance -= commitFeeMSat
}

// If the commitment has anchors, we must also subtract the anchor

This comment has been minimized.

Copy link
@halseth

halseth Dec 11, 2019

Author Collaborator

This is a bit awkward, but is a result of that the balances we store in the db has had their commit fee subtracted. should we do a db migration to change this?

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jan 17, 2020

Member

You want instead to store the balance on disk with the anchor output size already subtracted?

// and commitment point. The keys are derived differently depending on the type
// of channel, and whether the commitment transaction is ours or the remote
// peer's.
func DeriveCommitmentKeys(commitPoint *btcec.PublicKey,

This comment has been minimized.

Copy link
@joostjager

joostjager Dec 11, 2019

Collaborator

Separate move from change in two commits, for easier review.

Copy link
Collaborator

joostjager left a comment

This look pretty good. The word anchor disappeared completely from lnwallet.Channel. I also see that there may be more to encapsulate, but the commitment building is probably the most important thing.

I still have that ideal in mind where there is a CommitmentBuilder interface and a separate instance for the anchor commit format. Those instances would be thin and build on a lot of shared building blocks. It is probably too much for now and this is already a good step in that direction.

lnwallet/commitment.go Show resolved Hide resolved
Copy link
Collaborator

cfromknecht left a comment

Quick skim, cool pr :)

input/script_utils.go Outdated Show resolved Hide resolved
input/script_utils.go Outdated Show resolved Hide resolved
input/script_utils.go Outdated Show resolved Hide resolved
input/script_utils.go Outdated Show resolved Hide resolved
input/script_utils.go Outdated Show resolved Hide resolved
input/script_utils.go Show resolved Hide resolved
input/size.go Show resolved Hide resolved
input/size.go Outdated Show resolved Hide resolved
lnwallet/reservation.go Outdated Show resolved Hide resolved
@halseth halseth force-pushed the halseth:pluggable-anchors-lnwallet branch from 5f89aad to 819a5dd Dec 12, 2019
@halseth halseth added the anchors label Dec 13, 2019
@halseth halseth force-pushed the halseth:pluggable-anchors-lnwallet branch 2 times, most recently from b55825f to 0bb5f08 Dec 13, 2019
@halseth halseth force-pushed the halseth:pluggable-anchors-lnwallet branch from 0bb5f08 to 62fe01f Jan 7, 2020
Copy link
Collaborator

joostjager left a comment

Some drive-by comments

lnwallet/commitment.go Outdated Show resolved Hide resolved
lnwallet/commitment.go Show resolved Hide resolved
lnwallet/commitment.go Outdated Show resolved Hide resolved
@@ -229,6 +229,18 @@ func CommitScriptToRemote(chanType channeldb.ChannelType, csvTimeout uint32,
}, nil
}

func InitiatorFee(chanType channeldb.ChannelType, commitWeight int64,

This comment has been minimized.

Copy link
@joostjager

joostjager Jan 7, 2020

Collaborator

Would there be a better term for miner_fee + anchors then "fee"? To prevent introducing a second definition of fee. Cost, overhead, ..?

This comment has been minimized.

Copy link
@halseth

halseth Jan 16, 2020

Author Collaborator

I like cost perhaps?

}

// Not found.
return nil

This comment has been minimized.

Copy link
@joostjager

joostjager Jan 7, 2020

Collaborator

Could make this stricter and return an error if the channel type says there must be anchors.

lnwallet/channel.go Outdated Show resolved Hide resolved
lnwallet/channel.go Show resolved Hide resolved
fundingmanager.go Outdated Show resolved Hide resolved
lnwallet/commitment.go Outdated Show resolved Hide resolved
input/script_utils.go Outdated Show resolved Hide resolved
@Roasbeef Roasbeef added this to the 0.10.0 milestone Jan 14, 2020
@Roasbeef Roasbeef added the v0.10 label Jan 14, 2020
@halseth halseth force-pushed the halseth:pluggable-anchors-lnwallet branch 3 times, most recently from 3661f7e to d17272b Jan 16, 2020
@halseth halseth marked this pull request as ready for review Jan 16, 2020
@halseth halseth requested a review from Roasbeef as a code owner Jan 16, 2020
Copy link
Member

Roasbeef left a comment

Solid PR! We're soo close 🤗

The one thing that seems to be missing from my run through is ensuring that we re-account for the value that we deducted for anchors each time we go to create a new commitment.

lnwallet/commitment.go Outdated Show resolved Hide resolved
lnwallet/commitment.go Show resolved Hide resolved
remoteBalance = theirBalance.ToSatoshis()
)
if cb.chanState.ChanType.HasAnchors() {
anchors := 2 * anchorSize

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jan 17, 2020

Member

totalAnchorValue?

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jan 17, 2020

Member

Also could very well just be lifted into a constant of its own.

theirBalance -= commitFeeMSat
}

// If the commitment has anchors, we must also subtract the anchor

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jan 17, 2020

Member

You want instead to store the balance on disk with the anchor output size already subtracted?

lnwallet/channel.go Show resolved Hide resolved
// genSweepTx generates a swepp of the senderCommitTx, and sets the
// sequence if locktime is true.
genSweepTx := func(locktime bool) {
prevOut := &wire.OutPoint{

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jan 17, 2020

Member

A worthy follow up PR would be also testing second-level HTLC aggregation.

input/script_utils.go Show resolved Hide resolved
fundingmanager.go Outdated Show resolved Hide resolved
lnwallet/reservation.go Show resolved Hide resolved
@@ -213,6 +219,16 @@ func NewChannelReservation(capacity, localFundingAmt btcutil.Amount,
)
}

// Similarly we ensure their balance is reasonable if we are not the
// initiator.
if !initiator && theirBalance.ToSatoshis() <= 2*DefaultDustLimit() {

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jan 17, 2020

Member

Where does this requirement come from?

This comment has been minimized.

Copy link
@halseth

halseth Jan 24, 2020

Author Collaborator

It is not a new requirement, it was just something I noticed wasn't checked. Made it a separate commit.

This comment has been minimized.

Copy link
@joostjager

joostjager Feb 14, 2020

Collaborator

Why 2 times the dust limit?

This comment has been minimized.

Copy link
@halseth

halseth Feb 18, 2020

Author Collaborator

From 10 lines above

This comment has been minimized.

Copy link
@joostjager

joostjager Feb 24, 2020

Collaborator

Isn't 10 lines above 2 times the anchor size?

This comment has been minimized.

Copy link
@halseth

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Mar 1, 2020

Collaborator

this is likely wrong for LTC, but so are most of these checks...

@Roasbeef Roasbeef added this to In progress in v0.10.0-beta via automation Jan 17, 2020
@Roasbeef Roasbeef moved this from In Progress to Review In Progress in v0.10.0-beta Jan 17, 2020
@halseth halseth force-pushed the halseth:pluggable-anchors-lnwallet branch 2 times, most recently from c472925 to fa13a63 Jan 23, 2020
halseth and others added 24 commits Mar 6, 2020
…witness script

We use the fact that we can tell whether the commit is local or remote
by inspecting the witness script. We cannot use the maturity delay
anymore, as we can have delayed to_remote outputs also now.

Co-authored-by: Joost Jager <joost.jager@gmail.com>
With this commitment type, we'll add extra anchor outputs to the
commitment transaction if the anchor channel type is active.
… type

Based on the channel type, the commitment weight will be calculated.
Defaults to disabled.
Since we are also going to use it for experimental new features.
To prepare for adding more commit types to test for basic channel
funding, we make the commit type an enum that gets its own set of
subtests.
If we are the initiator, we check that our starting balance after
subtracting fees are not less than two times the default dust limit.

This commit adds a similar check for the non-initiator case, checking
that the remote party has a starting balance of reasonable size.
If both nodes are signalling the feature, make all opened channels using
this type.
@halseth halseth force-pushed the halseth:pluggable-anchors-lnwallet branch from 00aa935 to b7885db Mar 9, 2020
Copy link
Member

Roasbeef left a comment

LGTM 📯

Excellent work @halseth and @joostjager!

We should also extend the integration tests to cover breach retribution for all the existing commitment types. This can be done as a follow up PR. I'll also put together a small follow up PR to enable SCB support as well.

// AnchorsRequired is a required feature bit that signals that the node
// requires channels to be made using commitments having anchor
// outputs.
AnchorsRequired FeatureBit = 1336

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Mar 10, 2020

Member

Rather it should be resolved before we cut a release with this included. Feature bits are easy to change after the fact.

// HtlcConfirmedScriptOverhead is the extra length of an HTLC script
// that requires confirmation before it can be spent. These extra bytes
// is a result of the extra CSV check.
HtlcConfirmedScriptOverhead = 3

This comment has been minimized.

Copy link
@Roasbeef
v0.10.0-beta automation moved this from Review In Progress to Merge Queue Mar 10, 2020
@Roasbeef Roasbeef merged commit 3dda93e into lightningnetwork:master Mar 10, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.007%) to 63.121%
Details
v0.10.0-beta automation moved this from Merge Queue to Done Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v0.10.0-beta
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.