watchtower: support taproot channel commitments #7733

ellemouton · 2023-05-28T07:30:43Z

In this PR, the watchtower server and client are updated to support backing up taproot channels.

This change is

saubyk

Concept ACK

Roasbeef

Nice work! Dig the prepatory refactors to smooth out the integration of this channel type to the tower, as well as others.

No major comments, but still need to get more up to speed with the latest state of the codebase, as has been a while since I've reviewed anything here.

One thing I think we'll want to do to is have any instances of a new taproot blob or commitment type instead be taprootStaging. That final script change may not actually go through, but we should make sure to hedge things out a bit.

watchtower/blob/commitments.go

watchtower/blob/type.go

watchtower/blob/commitments.go

watchtower/blob/justice_kit.go

watchtower/blob/justice_kit_test.go

Roasbeef · 2023-12-13T02:30:32Z

watchtower/wtmock/signer.go

+	// In case of a taproot output any signature is always a Schnorr
+	// signature, based on the new tapscript sighash algorithm.
+	if txscript.IsPayToTaproot(signDesc.Output.PkScript) {
+		sigHashes := txscript.NewTxSigHashes(


We've copied this over in a few places in the codebase, perhaps we can try to unify with an internal/mock package?

do you mean the entire mock signer? dont think we can do that since it would cause an import package between input and internal/mock.

I guess another option is just to extend input.MockSigner so that we can re-use it here?

Ahh true re import cycle, I think I ran into the same thing when adding musig2 awareness in the first place...

Perhaps we can just make a new issue here to attempt to conslidate the mock signers in a single place. We don't update it all that often, but it can def burn some dev time when something small changes, and one mock signer works, but not the other.

watchtower/wtclient/session_negotiator.go

server.go

Roasbeef

LGTM 🐟

Reviewable status: 0 of 33 files reviewed, 24 unresolved discussions (waiting on @ellemouton and @yyforyongyu)

watchtower/wtpolicy/policy.go line 127 at r4 (raw file):

// FeatureBits returns the watchtower feature bits required for the given
// policy.
func (p *Policy) FeatureBits() []lnwire.FeatureBit {

👍

lightninglabs-deploy · 2024-01-12T03:22:22Z

@yyforyongyu: review reminder

yyforyongyu

Very nice 🎉 Left a few nits and questions, my main comment is there seems to be some test cases that won't be hit because how we check channel types.

input/script_utils.go

yyforyongyu · 2024-01-18T11:05:52Z

watchtower/blob/justice_kit_packet.go

@@ -400,3 +415,222 @@ func (b *justiceKitPacketV0) decode(r io.Reader) error {

 	return nil
 }
+
+// justiceKitPacketV1 is lé Blob of Justice for taproot channels.


what is lé

it's a fancy-shamnsy way of saying "the" 😂

left this in cause that's the original way that Connor wrote it and I thought it was funny :) but it's already in the interface comment so will remove it here 😉

my two sats here: "le" is the French masculine word for "the". But because just "le" doesn't look very French, I assume the acute accent was added (and I believe all of this is from Sponge Bob if I'm not mistaken).

watchtower/blob/justice_kit_packet.go

yyforyongyu · 2024-01-18T11:13:32Z

watchtower/blob/justice_kit.go

+// JusticeKit interface.
+var _ JusticeKit = (*taprootJusticeKit)(nil)
+
+// newTaprootJusticeKit constructs a new anchorJusticeKit.


Suggested change

// newTaprootJusticeKit constructs a new anchorJusticeKit.

// newTaprootJusticeKit constructs a new taprootJusticeKit.

watchtower/blob/justice_kit.go

yyforyongyu · 2024-01-18T13:06:48Z

watchtower/wtpolicy/policy.go

@@ -114,26 +116,44 @@ type Policy struct {
 }

 // String returns a human-readable description of the current policy.
-func (p Policy) String() string {
+func (p *Policy) String() string {


why this change?

was just to make my IDE happy 😝 in general seems to be better to have pointer receivers to avoid some unexpected behaviour

turns out this actually does need to stay like this or else the log line doesnt print properly 🙃 so changed it back

itest/lnd_revocation_test.go

server.go

watchtower/wtclient/backup_task_internal_test.go

yyforyongyu · 2024-01-18T13:25:28Z

watchtower/wtpolicy/policy.go

 // IsAnchorChannel returns true if the session policy requires anchor channels.
-func (p Policy) IsAnchorChannel() bool {
+func (p *Policy) IsAnchorChannel() bool {


wondering whether we should restrict the method IsAnchorChannel, and all its occurrence, to only return true if this channel is anchor type, and not taproot type - should help to avoid misusing as we now have to always keep in mind to check IsTaproot first before check IsAnchor in a switch statement, maybe we should do this on ChannelType - future PRs tho.

so for this instance (ie, in towers alone) it does actually just mean anchors. So this will return false for a taproot blob type.

but agreed that the we should make this the standard everywhere :)

so for this instance (ie, in towers alone) it does actually just mean anchors. So this will return false for a taproot blob type.

Yeah I do notice that the feature bit is 101 and 11, tho not sure about how to properly define a feature bit, as it seems that technically if anchor is a feature, then taproot chan should be 111 since it has this feature - unknown implications here, but I prefer the end result here as we can clearly distinguish the two channels. Meanwhile this shouldn't be an issue for feature negotiation since both are lnd? I think if both sides understand and use the same defined feature bits we are fine, eg, they won't send us a 111.

yeah i guess it's slightly different in this tower protocol cause the tower only cares about commit types as a whole since this completely changes the justice kit etc.

Meanwhile this shouldn't be an issue for feature negotiation since both are lnd?

yeah - currently the code for this is the spec. So this is fine for 2 lnd nodes, yeah 👍

Factor out the building of the delay and revoke scripts from NewLocalCommitScriptTree so that they can be re-used later on.

This commit adds a new FlagTaprootChannel Flag which is then used to construct a new blob Type: TypeAltruistTaprootCommit. New watchtower feature bits are also added (4/5).

In preparation for the new Taproot commitment type being supported by watchtowers, we add a new justice packet type.

coderabbitai · 2024-01-18T16:34:11Z

Important

Auto Review Skipped

Auto reviews are limited to the following labels: llm-review. Please add one of these labels to enable auto reviews.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository.

To trigger a single review, invoke the @coderabbitai review command.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit-tests for this file.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit tests for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository from git and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit tests.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

ellemouton

Thanks @yyforyongyu!! 🚀

Updated!

ellemouton · 2024-01-18T15:55:00Z

watchtower/blob/justice_kit_packet.go

@@ -400,3 +415,222 @@ func (b *justiceKitPacketV0) decode(r io.Reader) error {

 	return nil
 }
+
+// justiceKitPacketV1 is lé Blob of Justice for taproot channels.


it's a fancy-shamnsy way of saying "the" 😂

ellemouton · 2024-01-18T15:56:17Z

watchtower/blob/justice_kit_packet.go

@@ -400,3 +415,222 @@ func (b *justiceKitPacketV0) decode(r io.Reader) error {

 	return nil
 }
+
+// justiceKitPacketV1 is lé Blob of Justice for taproot channels.


left this in cause that's the original way that Connor wrote it and I thought it was funny :) but it's already in the interface comment so will remove it here 😉

ellemouton · 2024-01-18T15:58:38Z

watchtower/blob/justice_kit.go

+// JusticeKit interface.
+var _ JusticeKit = (*taprootJusticeKit)(nil)
+
+// newTaprootJusticeKit constructs a new anchorJusticeKit.


watchtower/blob/commitments.go

watchtower/wtclient/backup_task_internal_test.go

ellemouton · 2024-01-18T16:15:48Z

watchtower/wtpolicy/policy.go

@@ -114,26 +116,44 @@ type Policy struct {
 }

 // String returns a human-readable description of the current policy.
-func (p Policy) String() string {
+func (p *Policy) String() string {


was just to make my IDE happy 😝 in general seems to be better to have pointer receivers to avoid some unexpected behaviour

ellemouton · 2024-01-18T16:17:47Z

watchtower/wtpolicy/policy.go

 // IsAnchorChannel returns true if the session policy requires anchor channels.
-func (p Policy) IsAnchorChannel() bool {
+func (p *Policy) IsAnchorChannel() bool {


so for this instance (ie, in towers alone) it does actually just mean anchors. So this will return false for a taproot blob type.

but agreed that the we should make this the standard everywhere :)

server.go

itest/lnd_revocation_test.go

yyforyongyu

LGTM! Welcome towers to the taproot gang 😎

yyforyongyu · 2024-01-19T08:59:20Z

watchtower/wtpolicy/policy.go

 // IsAnchorChannel returns true if the session policy requires anchor channels.
-func (p Policy) IsAnchorChannel() bool {
+func (p *Policy) IsAnchorChannel() bool {


so for this instance (ie, in towers alone) it does actually just mean anchors. So this will return false for a taproot blob type.

Yeah I do notice that the feature bit is 101 and 11, tho not sure about how to properly define a feature bit, as it seems that technically if anchor is a feature, then taproot chan should be 111 since it has this feature - unknown implications here, but I prefer the end result here as we can clearly distinguish the two channels. Meanwhile this shouldn't be an issue for feature negotiation since both are lnd? I think if both sides understand and use the same defined feature bits we are fine, eg, they won't send us a 111.

ellemouton added watchtower taproot chans labels May 28, 2023

ellemouton marked this pull request as draft May 28, 2023 07:30

saubyk added this to the v0.17.0 milestone May 30, 2023

ellemouton force-pushed the taprootTowers branch from b419d61 to 75f480f Compare May 31, 2023 08:23

ellemouton changed the base branch from simple-taproot-chans-staging to taprootTowerRef May 31, 2023 08:23

ellemouton force-pushed the taprootTowers branch from 75f480f to 3540cb0 Compare May 31, 2023 10:38

ellemouton self-assigned this May 31, 2023

saubyk approved these changes Jun 1, 2023

View reviewed changes

ellemouton mentioned this pull request Jun 7, 2023

[5/? ] - lnwallet: add taproot funding support to the internal wallet flow (reservations) #7344

Merged

7 tasks

saubyk modified the milestones: v0.17.0, v0.17.1 Jun 15, 2023

saubyk added the P1 MUST be fixed or reviewed label Aug 8, 2023

saubyk modified the milestones: High Priority, v0.18.0 Aug 8, 2023

ellemouton force-pushed the taprootTowerRef branch from e83d218 to 9dfbdfd Compare August 17, 2023 18:34

ellemouton force-pushed the taprootTowers branch from 3540cb0 to ebef7dc Compare August 17, 2023 18:36

ellemouton changed the base branch from taprootTowerRef to master August 23, 2023 10:06

ellemouton force-pushed the taprootTowers branch 3 times, most recently from ea83003 to af36303 Compare August 29, 2023 07:20

ellemouton marked this pull request as ready for review August 29, 2023 07:24

ellemouton changed the title ~~[WIP] watchtower: support taproot channel commitments~~ watchtower: support taproot channel commitments Aug 29, 2023

This was referenced Aug 30, 2023

wtclient: Tower Client Multiplexer #7702

Merged

watchtower+refactor: CommitmentType and JusticeKit interface #7736

Merged

ellemouton force-pushed the taprootTowers branch 3 times, most recently from e885b37 to 128fec5 Compare November 28, 2023 12:08

ellemouton force-pushed the taprootTowers branch from 128fec5 to dab0404 Compare December 5, 2023 10:32

ellemouton requested a review from Roasbeef December 6, 2023 06:46

Roasbeef requested changes Dec 13, 2023

View reviewed changes

ellemouton force-pushed the taprootTowers branch from dab0404 to f34a718 Compare December 13, 2023 15:47

ellemouton requested a review from Roasbeef January 2, 2024 08:35

ellemouton force-pushed the taprootTowers branch from f34a718 to 04c245c Compare January 4, 2024 14:15

saubyk requested a review from yyforyongyu January 4, 2024 15:27

Roasbeef approved these changes Jan 5, 2024

View reviewed changes

yyforyongyu requested changes Jan 18, 2024

View reviewed changes

ellemouton added 6 commits January 18, 2024 17:56

input: extract to_local script builders

fde982a

Factor out the building of the delay and revoke scripts from NewLocalCommitScriptTree so that they can be re-used later on.

watchtower: add new TaprootCommit Type and flag

20a1076

This commit adds a new FlagTaprootChannel Flag which is then used to construct a new blob Type: TypeAltruistTaprootCommit. New watchtower feature bits are also added (4/5).

watchtower/blob: add justiceKitPacketV1 encoding

0ec3f31

In preparation for the new Taproot commitment type being supported by watchtowers, we add a new justice packet type.

watchtower/blob: add taprootJusticeKit implementation

d84a98e

watchtower/blob: add taproot Commitment type

5960253

watchtower/lookout: make justice desciptor taproot ready

c50aa10

ellemouton force-pushed the taprootTowers branch from 04c245c to 9f45c4b Compare January 18, 2024 16:34

ellemouton commented Jan 18, 2024

View reviewed changes

yyforyongyu approved these changes Jan 19, 2024

View reviewed changes

ellemouton added 5 commits January 19, 2024 15:33

watchtower/wtclient: prep client for taproot towers

660f1f3

lnrpc+lncli: update wtclientrpc for taproot towers

3cb194c

server: register a taproot tower client

f12cc12

itest: add taproot tower itest

55ee01a

docs: add release note entry for 7733

2a61c91

ellemouton force-pushed the taprootTowers branch from 9f45c4b to 2a61c91 Compare January 19, 2024 13:33

ellemouton merged commit 0a29b37 into lightningnetwork:master Jan 19, 2024
24 of 25 checks passed

ellemouton deleted the taprootTowers branch January 19, 2024 20:55

	// newTaprootJusticeKit constructs a new anchorJusticeKit.
	// newTaprootJusticeKit constructs a new taprootJusticeKit.

watchtower: support taproot channel commitments #7733

watchtower: support taproot channel commitments #7733

Conversation

ellemouton commented May 28, 2023 • edited by Roasbeef Loading

saubyk left a comment

Choose a reason for hiding this comment

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef left a comment

Choose a reason for hiding this comment

lightninglabs-deploy commented Jan 12, 2024

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderabbitai bot commented Jan 18, 2024

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

CodeRabbit Discord Community

ellemouton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellemouton commented May 28, 2023 •

edited by Roasbeef

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)