[2/?] - input: add taproot chan scripts, control block logic, and spending routines by Roasbeef · Pull Request #7333 · lightningnetwork/lnd

Roasbeef · 2023-01-17T04:12:47Z

Change Description

In this commit, we add the current draft set of taproot scripts, control block handling logic, and also spending routines.

Reviewers will want to be familiar with the current spec draft before diving in: https://github.com/Roasbeef/lightning-rfc/blob/simple-taproot-chans/bolt-simple-taproot.md

This PR is a draft PR, as it still needs tests for all the new routines (they work in #6877) and also the witness estimate constants to be updated.

Steps to Test

Steps for reviewers to follow to test the change.

Pull Request Checklist

Testing

Your PR passes all CI checks.
Tests covering the positive and negative (error paths) are included.
Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
Commits follow the Ideal Git Commit Structure.
Any new logging statements use an appropriate subsystem and logging level.
There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

guggero

Was curious about the PR and took a quick glance at it.
Noticed a few things that I wanted to point out, this is by no means a full review.

guggero

Did a first more detailed pass. Amazing work! Very easy to read diff and code!
Given the number of small things that are off (witness stack indexes, sighash flags present), I guess it would be nice to have unit tests that cover at least some very basic paths. Let me know if I should help out with these.

Roasbeef · 2023-02-04T13:13:22Z

Pushed up a new commit that adds exhaustive tests to the new scripts. This tracks the current draft PR, which'll be updated soon to have some small changes (remove implicit CSV 0, etc). Removed it from draft now as unit tests are in place.

guggero

Last two commits look good, great unit tests!
Since there are still a few discussions open that need to be resolved in the spec as well, I'll wait with the final pass.

Just two things I noticed:

The TestTaprootReceiverHtlcSpend seems to be flaky, there seems to be a y coordinate odd issue
Running the unit tests with coverage, the following functions don't seem to be covered yet: (though I'm not sure if we are aiming for 100% unit test coverage on these new functions, as they'll be executed in the itests for sure)
- GenTaprootFundingScript
- TaprootSecondLevelHtlcScript
- TaprootCommitScriptToSelf
- TaprootCommitScriptToRemote
- TaprootOutputKeyAnchor

ellemouton

Finally did a full pass - Looking great! 🔥

I guess any spec changes/changes resulting from the discussion in this PR would only affect comments and variable names.

yyforyongyu · 2023-02-17T06:53:05Z

 	return witnessStack, nil
 }

+// SenderHTLCTapLeafTimeout returns the full tapscript leaf for the timeout


nit: change this to Offered... so it's easier to follow the specs.

yyforyongyu · 2023-02-17T09:28:18Z

 	return builder.Script()
 }

+// TODO(roasbeef): move all taproot stuff to new file?


yyforyongyu

Very thorough unit tests🙏 I think it's very close, my major question is the OP_SIZE check, whether it's still necessary.

guggero

LGTM 🎉

guggero · 2023-05-26T07:51:16Z

Yeah, my mental parser always throws a fit when seeing this 😅 I think mostly because the indentation isn't preserved, which is a whole new visual pattern to learn when scanning code...

guggero · 2023-05-26T07:53:25Z

IMO we can also disable that linter in general as there are good reasons for not running some tests in parallel some times.

yyforyongyu

🌊⛵️🏖

yyforyongyu · 2023-05-26T08:13:32Z

This is not fixed yet, can be confusing for future development.

In this commit, we add a helper function to take a taproot output key and turn it into a v1 witness program.

In this commit, we add GenTaprootFundingScript, which'll return the taproot pkScript and output for a taproot+musig2 channel. This uses musig2 key aggregation with sorting activated. The final key produced uses a bip86 tweak, meaning that the output key provably doesn't commit to any script path. In the future, we may want to permit this, as then it allows for a greater degree of programmability of the funding output.

Unlike the old HTLC scripts, we now need to handle the various control block interactions. As is, we opt to simply re-compute the entire tree when needed, as the tree only has two leaves.

In this commit, we restore usage of the NUMS key for the to remote output, as this allows a remote party to scan the chain in order to find their remote output that in emergency recovery scenarios.

In this commit, we modify the to_local script to use a script path for the revocation scenario. With this change, we ensure that the internal key is always revealed which means the anchor outputs can still always be swept.

We undo the prior hack here to make the final script more readable. The difference is just 1 byte between the two.

Crypt-iQ · 2023-05-26T16:07:34Z

nit: no longer keyspend

Crypt-iQ · 2023-05-26T16:20:47Z

Is there some way that we can have a test or linter job that yells if somebody ever tries to change this?

Crypt-iQ · 2023-05-31T17:46:59Z

+// TaprootAnchorSpend constructs a valid witness allowing a node to sweep their
+// anchor output.
+func TaprootAnchorSpend(signer Signer, signDesc *SignDescriptor,
+	revokeTx *wire.MsgTx) (wire.TxWitness, error) {


nit: can't spend anchor w/ revoke

Crypt-iQ · 2023-05-31T18:03:51Z

 // computed as:
 //
-//	delay_control_block = (output_key_y_parity | 0xc0) || revocationpubkey
+//	delay_control_block = (output_key_y_parity | 0xc0) || taproot_nums_key


missing the opposite leaf

guggero reviewed Jan 17, 2023

View reviewed changes

Comment thread input/script_utils.go Outdated

Comment thread input/script_utils.go Outdated

Comment thread input/script_utils.go Outdated

hieblmi reviewed Jan 17, 2023

View reviewed changes

Comment thread input/script_utils.go Outdated

saubyk requested a review from Crypt-iQ January 17, 2023 18:06

saubyk added this to the v0.16.0 milestone Jan 17, 2023

saubyk assigned Roasbeef Jan 17, 2023

hieblmi reviewed Jan 17, 2023

View reviewed changes

Comment thread input/script_utils.go Outdated

saubyk added taproot funding Related to the opening of new channels with funding transactions on the blockchain labels Jan 19, 2023

Roasbeef changed the title ~~input: add taproot chan scripts, control block logic, and spending routines~~ [2/?] - input: add taproot chan scripts, control block logic, and spending routines Jan 19, 2023

Roasbeef added the taproot chans label Jan 19, 2023

Roasbeef force-pushed the simple-taproot-chans-scripts branch from ec40eea to c6b0367 Compare January 19, 2023 04:02

Roasbeef mentioned this pull request Jan 19, 2023

[4/?] - input+lnwallet: prepare input package for funding logic, add new MusigSession abstraction #7340

Merged

7 tasks

guggero self-requested a review January 19, 2023 08:43

ellemouton reviewed Jan 19, 2023

View reviewed changes

Comment thread input/script_utils.go Outdated

Comment thread input/script_utils.go Outdated

Comment thread input/script_utils.go Outdated

Comment thread input/script_utils.go Outdated

Crypt-iQ reviewed Jan 19, 2023

View reviewed changes

guggero reviewed Jan 20, 2023

View reviewed changes

Roasbeef force-pushed the simple-taproot-chans-scripts branch from c6b0367 to 1d1ab0b Compare January 25, 2023 04:43

ellemouton reviewed Jan 26, 2023

View reviewed changes

Comment thread input/script_utils.go Outdated

Comment thread input/script_utils.go Outdated

Comment thread input/script_utils.go Outdated

Comment thread input/script_utils.go Outdated

Comment thread input/script_utils.go Outdated

joostjager reviewed Jan 31, 2023

View reviewed changes

Comment thread input/script_utils.go Outdated

Roasbeef marked this pull request as ready for review February 4, 2023 13:12

Roasbeef force-pushed the simple-taproot-chans-scripts branch from 3525b86 to ff278d8 Compare February 4, 2023 13:14

guggero self-requested a review February 6, 2023 09:19

guggero reviewed Feb 8, 2023

View reviewed changes

ellemouton reviewed Feb 8, 2023

View reviewed changes

Comment thread input/script_utils.go Outdated

Comment thread input/script_utils.go Outdated

Comment thread input/script_utils.go Outdated

Crypt-iQ reviewed Feb 9, 2023

View reviewed changes

saubyk modified the milestones: v0.16.0, v0.16.1 Feb 14, 2023

yyforyongyu reviewed Feb 17, 2023

View reviewed changes

ellemouton mentioned this pull request Mar 7, 2023

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

Merged

7 tasks

yyforyongyu reviewed May 25, 2023

View reviewed changes

Comment thread input/script_utils.go Outdated

Comment thread input/script_utils.go Outdated

Comment thread input/taproot_test.go Outdated

Comment thread input/taproot_test.go Outdated

Comment thread input/script_utils.go Outdated

Roasbeef force-pushed the simple-taproot-chans-scripts branch 5 times, most recently from 048e4c7 to 507f101 Compare May 26, 2023 01:52

guggero approved these changes May 26, 2023

View reviewed changes

yyforyongyu added the no-changelog label May 26, 2023

yyforyongyu approved these changes May 26, 2023

View reviewed changes

Roasbeef added 18 commits May 26, 2023 16:23

input: add PayToTaprootScript helper func

5f16e8e

In this commit, we add a helper function to take a taproot output key and turn it into a v1 witness program.

input: add TaprootCommitScriptToSelf for taproot to self script

fbdc28e

input: add TaprootCommitScriptToRemote for taproot to remote script

578a16a

input: add TaprootOutputKeyAnchor for taproot anchor outputs

99066d7

input: add tapscript utils for the sender HTLC script

84b36ec

Unlike the old HTLC scripts, we now need to handle the various control block interactions. As is, we opt to simply re-compute the entire tree when needed, as the tree only has two leaves.

input: add spending funcs for taproot sender HTLCs

8eb9490

input: add taproot script funcs for receiver HTLCs

5c10f28

input: add spending funcs for taproot receiver HTLC ctrl blocks

006113a

input: add taproot second level HTLC scripts

3e59b1b

input: add spending funcs for second level HTLC tapscript ctrl blocks

9a5151f

input: add new maybeAppendSighashType helper func

9485cc0

input: add exhaustive unit tests for new taproot scripts

110c29a

input: restore usage of NUMS key for to_remote output

6aca853

In this commit, we restore usage of the NUMS key for the to remote output, as this allows a remote party to scan the chain in order to find their remote output that in emergency recovery scenarios.

input: fix linter errors

7ef2306

input: use explicit CSV 1 script for to remote output

34a9cfb

We undo the prior hack here to make the final script more readable. The difference is just 1 byte between the two.

input: eliminate CSV trick for HTLC outputs

388a70c

Roasbeef force-pushed the simple-taproot-chans-scripts branch from 507f101 to 388a70c Compare May 26, 2023 23:25

Roasbeef merged commit 12463c1 into lightningnetwork:simple-taproot-chans-staging May 26, 2023

Crypt-iQ reviewed May 31, 2023

View reviewed changes

Conversation

Roasbeef commented Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Description

Steps to Test

Pull Request Checklist

Testing

Code Style and Documentation

Uh oh!

guggero left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guggero left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Roasbeef commented Feb 4, 2023

Uh oh!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

ellemouton left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Roasbeef commented Jan 17, 2023 •

edited

Loading