[anchor] Pluggable commitments by halseth · Pull Request #3829 · lightningnetwork/lnd

halseth · 2019-12-12T14:57:55Z

This lays the groundwork for making it easy to introduce anchor commitments.

Replaces #3773

joostjager

Great improvement in the structure of the code and separation of responsibilities. I definitely feel more comfortable building anchor outputs on top of this.

With regards to naming, it can get quite confusing here and there with the definition of local/remote. Maybe in some cases, our and their may be more clear.

And I keep thinking that it should be possible to also convert the remaining commitment.go funcs to methods, perhaps on a lighter struct. Just to prevent mistakes with passing in parameters. I don't know if it is worth the effort at this point though.

cfromknecht · 2019-12-17T01:16:07Z

idt we can actually remove this since these values are persisted. may need to keep this behavior, but funnel all new channels into the consolidated logic

Where are they persisted exactly?

What we can do is just define it there (or change the name here to indicate it should not be used anymore), and convert it to the common type when reading it from disk.

breacharbiter and nursery for sure, probably a few other places.

What we can do is just define it there (or change the name here to indicate it should not be used anymore), and convert it to the common type when reading it from disk.

would need to see in how many places the serialization logic lives, but that's definitely one path. or we just leave the switch statements in place. since the tweakless-ness is now signaled through the sign descriptor, it might not be so bad

Had the same thought when I saw this, IIRC they're also persisted in some tower logic as well. Even independent of that, it's a useful witness type, as I may want to pass the very same sign descriptor for the two commitment types, an example being in the future if we opt to allow existing channels to switch over to this new witness type.

halseth · 2019-12-19T09:47:48Z

With regards to naming, it can get quite confusing here and there with the definition of local/remote. Maybe in some cases, our and their may be more clear.

I did a sweep to find a few places where this could be made more clear. Opted for ours/theirs in cases of static perspective (where we are always handling keys, outputs, commitments from our local node perspective), and local/remote for the dynamic perspective (where we are sharing logic from both perspectives).

Obviously this naming is used interchangeably throughout the codebase, so it was an effort way to large for this PR to change it everywhere, but would be nice to decide on a naming scheme and stick to it going forward.

I did the change in the parts already touched by this PR and for the ChannelConfigs:
0785952
7f7449d
03a6d37
4f02cde
80f4ece

lmk what you think!

halseth · 2019-12-19T14:15:27Z

With regards to naming, it can get quite confusing here and there with the definition of local/remote. Maybe in some cases, our and their may be more clear.

I did a sweep to find a few places where this could be made more clear. Opted for ours/theirs in cases of static perspective (where we are always handling keys, outputs, commitments from our local node perspective), and local/remote for the dynamic perspective (where we are sharing logic from both perspectives).

Obviously this naming is used interchangeably throughout the codebase, so it was an effort way to large for this PR to change it everywhere, but would be nice to decide on a naming scheme and stick to it going forward.

I did the change in the parts already touched by this PR and for the ChannelConfigs:
0785952
7f7449d
03a6d37
4f02cde
80f4ece

lmk what you think!

Ended up reverting most of these changes, since it made a large PR even larger. I think we should agree on a proper way of naming things, and do a PR that does only that change if wanted.

cfromknecht

@halseth did another pass, overall pr looks pretty complete. only q rn is how to deal with existing CommitmentNoDelayTweakless witness types in the database

cfromknecht · 2019-12-19T23:09:09Z

 		}

 		witnessType := input.CommitmentNoDelay
-		if tweakless {


seems we don't test tweakless at all now?

cfromknecht · 2019-12-19T23:10:47Z

breacharbiter and nursery for sure, probably a few other places.

What we can do is just define it there (or change the name here to indicate it should not be used anymore), and convert it to the common type when reading it from disk.

would need to see in how many places the serialization logic lives, but that's definitely one path. or we just leave the switch statements in place. since the tweakless-ness is now signaled through the sign descriptor, it might not be so bad

halseth · 2019-12-20T08:31:03Z

Removed the last commit, we can move that discussion to #3864, as it is not essential for anchor progress.

cfromknecht · 2019-12-20T09:17:04Z

-	localKey := lc.localChanCfg.MultiSigKey.PubKey.SerializeCompressed()
-	remoteKey := lc.remoteChanCfg.MultiSigKey.PubKey.SerializeCompressed()
+	localKey := lc.channelState.LocalChanCfg.MultiSigKey.PubKey.SerializeCompressed()
+	remoteKey := lc.channelState.RemoteChanCfg.MultiSigKey.PubKey.SerializeCompressed()


nit: long lines?

FIxed: 907d1f3

cfromknecht · 2019-12-20T09:17:31Z

-	ourKey := lc.localChanCfg.MultiSigKey.PubKey.SerializeCompressed()
-	theirKey := lc.remoteChanCfg.MultiSigKey.PubKey.SerializeCompressed()
+	ourKey := lc.channelState.LocalChanCfg.MultiSigKey.PubKey.SerializeCompressed()
+	theirKey := lc.channelState.RemoteChanCfg.MultiSigKey.PubKey.SerializeCompressed()


nit: also long line

Fixed: 907d1f3

cfromknecht · 2019-12-20T09:23:31Z

 		RevocationKey: c.revPK,
-		NoDelayKey:    c.toLocalPK,
-		DelayKey:      c.toRemotePK,
+		RemoteKey:     c.toLocalPK,


nice 👍 it case it isn't clear (from the lack of comments 😅) it's important to flip local and reverse before giving this to the wtclient since the logic is from the opposite perspective.

joostjager · 2019-12-20T10:27:04Z

-	} else {
-		dustLimit = lc.channelState.LocalChanCfg.DustLimit
+	// Actually generate unsigned commitment transaction for this view.
+	commitView, err := lc.commitBuilder.createCommitmentTx(


It seems that we use commitView for two different "views"? There is also fetchCommitmentView that returns a commitment

Yeah, true. Better name commitTxInfo perhaps?

It seems that the problem is that the type commitment is actually a commitmentView and that commitmentView is not much more than a commitment tx. We should probably swap the names.

At least renaming the local var here would be useful.

Fixed: 79c7f56

joostjager · 2019-12-20T10:32:39Z

+
+		// If this is a tweakless commitment, then we can safely blank
+		// out the commitment tweak, as it isn't needed.
+		keyRing.LocalCommitKeyTweak = nil


This confuses me. Why can we nil the tweak of the local outputs, which is tweaked regardless of the chan type?

Maybe we should swap the parameters of DeriveCommitmentKeys in the breach and remote force close contexts?

Reverted back to original change, only nilling when not our commit.

joostjager

Reviewed 8a93da4595eecfbd57ef941045b26f1aef337b00

joostjager · 2020-01-02T12:25:03Z

-	} else {
-		dustLimit = lc.channelState.LocalChanCfg.DustLimit
+	// Actually generate unsigned commitment transaction for this view.
+	commitView, err := lc.commitBuilder.createCommitmentTx(


It seems that the problem is that the type commitment is actually a commitmentView and that commitmentView is not much more than a commitment tx. We should probably swap the names.

At least renaming the local var here would be useful.

joostjager

LGTM

cfromknecht

did a final pass, only minor comments. otherwise LGTM! 👍

+1 for keeping local/remote instead of ours/theirs

cfromknecht · 2020-01-03T22:57:14Z

this is indeed quite confusing that it doesn't switch perspectives like everything else, esp if isOurs is passed into the derivation. probably not worth fixing rn tho

Yeah, this led to some confusion during review.

I also tried switching the perspective always, but that turned out to be a large change since it will ripple through many parts of the codebase, so felt it was better to leave it as-is for now, with some clarifying commentary.

It was incorrectly stated that the commitment balance was before subctracting the commit fee, which led to some confusion.

It has no dependency on LightningChannel

Instead of passing delays and dustlimits separately, we pass the correct channel config to CreateCommitTx from the POV of the local party that owns the commit tx. To make it more clear which commitment we are actually creating, we rename variables to denote local and remote, to prepare for the case when both outputs might be delayed.

PURE CODE MOVE: Moving createCommitmentTx, CreateCommitTx, createStateHintObfuscator, CommitmentKeyRing, DeriveCommitmentKeys, addHTLC, genHtlcScripts We move the methods and structs to a new file commitment.go in preparation for defining all the logic that is dependent on the channel type in this new file.

We define a new struct CommitmentBuilder that will be used to craft the final commitment transaction based on the current active channel type.

createCommitmentTx would earlier mutate the passed commitment struct after evaluating the htlc view and calculating the final balances, which was confusing since the balances are supposed to only be *after* subtracting fees. Instead we take the needed parameters as arguments, and return the final balances, tx and fee to populate the commitment struct in a proper way.

Since both parties are going to have their ouputs delayed, we move way from the DelayKey naming, and instead use ToLocalKey and ToRemoteKey.

We abstract away how keys are generated for the different channel types types (currently tweak(less)). Intention is that more of the logic that is unique for each commitment type lives in commitment.go, making the channel state machine oblivious to the keys and outputs being created on the commitment tx for a given channel state.

To make the channel state machine less concerned about the type of commitment, we nil the local tweak when creating the keyring, depending on the commitment type.

Also update the WitnessScript doc to note it should be set also for p2wkh.

Based on the current channel type, we derive the script used for the to_remote output. Currently only the unencumbered p2wkh type is used, but that will change with upcoming channel types.

When creating the keyring, the tweak is already calculated in the remote commitment case. We add the calculation also for our own commitment, so we can use it in all cases without deriving the tweak.

halseth requested a review from joostjager December 12, 2019 14:57

joostjager reviewed Dec 13, 2019

View reviewed changes

Comment thread lnwallet/commitment.go Outdated

Comment thread lnwallet/commitment.go Outdated

halseth force-pushed the pluggable-commitments-lnwallet branch from 7988c13 to 8997dbd Compare December 13, 2019 09:33

halseth marked this pull request as ready for review December 13, 2019 09:34

halseth requested review from Roasbeef and cfromknecht as code owners December 13, 2019 09:34

halseth added the anchors label Dec 13, 2019

halseth added this to the 0.9.0 milestone Dec 13, 2019

halseth mentioned this pull request Dec 13, 2019

[anchors] Pluggable commitments #3773

Closed

halseth force-pushed the pluggable-commitments-lnwallet branch from 8997dbd to d312255 Compare December 13, 2019 10:06

halseth mentioned this pull request Dec 13, 2019

[anchor] pluggable anchor commitments #3821

Merged

8 tasks

halseth force-pushed the pluggable-commitments-lnwallet branch from d312255 to 59cfcd9 Compare December 13, 2019 10:35

joostjager added the v0.9.0 label Dec 16, 2019

joostjager self-requested a review December 17, 2019 11:47

joostjager reviewed Dec 17, 2019

View reviewed changes

cfromknecht reviewed Dec 18, 2019

View reviewed changes

halseth force-pushed the pluggable-commitments-lnwallet branch from 59cfcd9 to 80f4ece Compare December 19, 2019 09:35

halseth requested review from cfromknecht and joostjager December 19, 2019 09:47

halseth force-pushed the pluggable-commitments-lnwallet branch 2 times, most recently from 2d5ef02 to e37e62f Compare December 19, 2019 14:14

halseth force-pushed the pluggable-commitments-lnwallet branch from e37e62f to 2703a10 Compare December 19, 2019 14:35

cfromknecht reviewed Dec 19, 2019

View reviewed changes

halseth force-pushed the pluggable-commitments-lnwallet branch from 2703a10 to d808d13 Compare December 20, 2019 08:29

halseth mentioned this pull request Dec 20, 2019

Remove tweakless #3864

Closed

cfromknecht approved these changes Dec 20, 2019

View reviewed changes

joostjager reviewed Dec 20, 2019

View reviewed changes

joostjager reviewed Jan 2, 2020

View reviewed changes

halseth force-pushed the pluggable-commitments-lnwallet branch 2 times, most recently from 0e58bf8 to 1281443 Compare January 3, 2020 11:40

joostjager approved these changes Jan 3, 2020

View reviewed changes

cfromknecht approved these changes Jan 3, 2020

View reviewed changes

halseth force-pushed the pluggable-commitments-lnwallet branch 2 times, most recently from 5914f8d to 3dfeeab Compare January 6, 2020 10:40

halseth added 8 commits January 6, 2020 11:42

channeldb+lnwallet: note that balance is after subtracting commit fee

2c46640

It was incorrectly stated that the commitment balance was before subctracting the commit fee, which led to some confusion.

lnwallet: remove unused initiator param to CreateCooperativeCloseTx

f001900

lnwallet: make addHtlc non-LightningChannel method

e2b050a

It has no dependency on LightningChannel

lnwallet: remove duplicate chanCfg fields, use channelState

1a4f81e

lnwallet: make fundingTxIn not depend on LightningWallet

13a108e

lnwallet: create CommitmentBuilder

613d771

We define a new struct CommitmentBuilder that will be used to craft the final commitment transaction based on the current active channel type.

halseth force-pushed the pluggable-commitments-lnwallet branch from 3dfeeab to a7dfc97 Compare January 6, 2020 10:42

halseth added 9 commits January 6, 2020 12:07

lnwallet/commitment: clarify keyring key ownership

7b9c549

lnwallet: rename DelayKey->ToLocalKey, NoDelayKey->ToRemoteKey

4fde312

Since both parties are going to have their ouputs delayed, we move way from the DelayKey naming, and instead use ToLocalKey and ToRemoteKey.

lnwallet: nil single tweak when creating keyring

f92c7a3

To make the channel state machine less concerned about the type of commitment, we nil the local tweak when creating the keyring, depending on the commitment type.

input: update SignDescriptor doc to note only segwit is supported

9b5809a

Also update the WitnessScript doc to note it should be set also for p2wkh.

lnwallet: use channel type to derive remote script

a56ed72

Based on the current channel type, we derive the script used for the to_remote output. Currently only the unencumbered p2wkh type is used, but that will change with upcoming channel types.

input: remove duplicate commit weight constant

3711597

lnwallet/channel: reuse derived SingleTweak on local force close

9c3218c

When creating the keyring, the tweak is already calculated in the remote commitment case. We add the calculation also for our own commitment, so we can use it in all cases without deriving the tweak.

halseth force-pushed the pluggable-commitments-lnwallet branch from a7dfc97 to 9c3218c Compare January 6, 2020 11:09

halseth merged commit 3aca9d2 into lightningnetwork:master Jan 7, 2020

Roasbeef mentioned this pull request Mar 10, 2020

Add anchor output to the commitment transaction #3587

Closed

joostjager mentioned this pull request Mar 10, 2020

cnct: anchor output sweeping and fee bumping #3758

Merged

halseth mentioned this pull request Mar 11, 2020

[anchors] lnrpc: add commitment_type to listchannels #4068

Merged

Conversation

halseth commented Dec 12, 2019

Uh oh!

Uh oh!

Uh oh!

joostjager left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

halseth commented Dec 19, 2019

Uh oh!

halseth commented Dec 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cfromknecht left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halseth commented Dec 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

joostjager left a comment •

edited

Loading

halseth commented Dec 19, 2019 •

edited

Loading