[6/?] - lnwallet+chancloser: update channel state machine and co-op close for musig2 flow #7345

Roasbeef · 2023-01-20T04:02:39Z

Change Description

In this PR, we update the channel state machine to be aware of the new musig2 flow. Along the way, we update some util functions to ensure we can use them to generate scripts/transactions for both segwit v0 and v1 channels.

In this new flow, each time we send a signature, we sig a partial sig along with the nonce that we used to sign. Upon receiving that signature, the remote party will use the nonce to complete their local session (for that state).

Each time a signature is received, a party will generate another nonce that'll be used to create their next local state. This next local/verification nonce is then sent alongside the revoke_and_ack message. Upon receiving the revocation, the party that originally sent the sig, can then use the new local nonce to prep the session for the next state.

We also update the co-op close state machine to be proper taproot aware. This involves setting the new nonce and partial signatures fields in the shutdown and closing signed messages. The other big change here is that we'll short circuit most of the co-op close flow. Rather than attempt a "negotiation", we'll just have the responder accept w/e the initiator sends, as they're the ones that pay the fees in the end.

In the next PR, we'll hook the new reservation flow up to the funding manager, and fill out the other functionality needed for the other sub-systems to be aware of the new channel type.

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.

lnwallet/channel.go

lnwallet/chancloser/chancloser.go

lnwallet/commitment.go

lnwallet/channel.go

yyforyongyu · 2023-02-21T11:48:18Z

lnwallet/channel.go

@@ -4462,11 +4462,30 @@ func genHtlcSigValidationJobs(localCommitmentView *commitment,
 					return nil, err
 				}

+				htlcAmt := int64(htlc.Amount.ToSatoshis())
+
+				if chanType.IsTaproot() {


I wonder if it's possible to only have this taproot check in a single place, quite challenging i guess. atm it's scattered in all the "components", like CreateHtlcSuccessTx also does this check.

Yeah it's a bit scattered right now. In other PR @joostjager brought up the idea we initially had to sort of split things out into more of a set of "builder" implementations of a new interface. The idea then is that we'd init the commitment/HTLC builder once, then call into that. That way we can remove all the branching we have for taproot vs not. It's a bigger change/refactor though, so happy to pursue it, but I think makes sense to defer for another PR series.

The other upcoming change we have is the taproot assets set of changes, which'll add yet another channel type, which brings its own unique attriobutes along. I think this would be a good time to make such a change.

lnwallet/channel.go

ellemouton

high level first pass comments 🤓

lnwallet/channel.go

lnwallet/transactions.go

lnwallet/commitment.go

lnwallet/channel.go

lnwallet/chancloser/chancloser.go

lnwallet/channel_test.go

lnwallet/transactions.go

lnwallet/channel.go

Roasbeef · 2023-07-14T02:05:58Z

Pushed up a new version, added tests for co-op close, and also tests for channel re-establish, PTAL!

ellemouton

Looking gooooood! Mostly non-blocking questions/comments. One blocking one in the co-op close commit.

EDIT: And then ofc the RBF change is still required.

ellemouton · 2023-07-14T08:48:55Z

lnwallet/transactions.go

+	if chanType.IsTaproot() {
+		taprootOutputKey, err := input.TaprootSecondLevelHtlcScript(
+			revocationKey, delayKey, csvDelay,
+		)
+		if err != nil {
+			return nil, err
+		}
+
+		pkScript, err = input.PayToTaprootScript(taprootOutputKey)
+		if err != nil {
+			return nil, err
+		}
+


no longer need this since the switch now happens in SecondLevelHtlcScript :)

ellemouton · 2023-07-14T08:55:01Z

lnwallet/commitment.go

+		if err != nil {
+			return nil, err
+		}
+
 	default:
 		witnessScript, err = input.SecondLevelHtlcScript(


missing error check for this

ellemouton · 2023-07-14T12:38:35Z

lnwallet/channel.go

@@ -3299,7 +3299,7 @@ func genRemoteHtlcSigJobs(keyRing *CommitmentKeyRing,

 		// If the HTLC isn't dust, then we'll create an empty sign job


nit: commit message is a bit stale I think. The We also modify the sigpool to now use a input.Signature everywhere part was in 5e7a97c

ellemouton · 2023-07-14T13:18:17Z

lnwallet/channel.go

@@ -1319,12 +1319,16 @@ type LightningChannel struct {
 	// log is a channel-specific logging instance.
 	log btclog.Logger

+	// taprootNonceProducer is used to generate a shachain tree for the


nit: commit message a bit stale. This commit updates all the signature validation methods for the channel & not only the HTLC validation

lnwallet/channel.go

lnwallet/test_utils.go

peer/musig_chan_closer.go

ellemouton · 2023-07-18T10:31:37Z

lnwallet/chancloser/chancloser.go

+		case isTaproot && isInitiator && !feeMatchesOffer:
+			return nil, false, fmt.Errorf("fee rate for "+
+				"taproot channels was not accepted: "+
+				"sent %v, got %v",
+				c.idealFeeSat, remoteProposedFee)


should we send some kind of warning to the peer here?

also - related to earlier question - should we make sure to re-set our closing verification nonce here? to ensure when we try close again that we dont use same nonce (since MusigChanCloser struct is still the same I think. I dont think it gets refreshed)

Yeah we should actually send it back here: https://github.com/lightningnetwork/lnd/blob/master/peer/brontide.go#L3528-L3540

What's try again mean here? When we fail above, the chancloser is removed.

When we fail above, the chancloser is removed.

Indeed - missed this. Mah bad

lnwallet/channel.go

yyforyongyu

Reviewed this PR to get a better understanding of the following PRs, think we need to fix CI tho.

yyforyongyu · 2023-07-18T10:13:34Z

channeldb/channel.go

+// another musig2 session. In order to permit our implementation to not have to
+// write any secret nonce state to disk, we'll use the _next_ shachain
+// pre-image as our primary randomness source. When used to generate the nonce
+// again to broadcast our commitment hte current height will be used.


Suggested change

// again to broadcast our commitment hte current height will be used.

// again to broadcast our commitment the current height will be used.

htlcswitch/link.go

yyforyongyu · 2023-07-18T10:32:53Z

lnwallet/chancloser/chancloser.go

-		// We'll compare the proposed total fee, to what we've proposed during
-		// the negotiations. If it doesn't match any of our prior offers, then
-		// we'll attempt to ratchet the fee closer to
+		// If this is a taproot channel, then it MUST have a partial


Something to do in the future, a general pattern we can have is case msg: processMsg or handleMsg, eg, here we'd have processCloseFeeNegotition to avoid the ever-growing long method and it's also easier to reason about it and test.

yyforyongyu · 2023-07-18T10:58:11Z

lnwallet/channel.go

+// NewCommitState wraps the various signatures needed to properly
+// propose/accept a new commitment state. This includes the signer's nonce for
+// musig2 channels.
+type NewCommitState struct {


maybe NextCommitState? New sounds like you are initializing a struct here.

lnwallet/channel_test.go

Crypt-iQ · 2023-07-19T13:24:28Z

lnwallet/chancloser/chancloser_test.go

+
+	// If we use the original offer, then Alice should accept this message,
+	// and finalize the shutdown process. We expect a message here as Alice
+	// will echo back the final message.


the spec doesn't explicitly say that we're allowed to echo back the final message -- this came up when we were testing zero-conf with LDK. I don't think we should change this here, but figured I'd point out

lnwallet/chancloser/chancloser_test.go

lnwallet/channel_test.go

We need to export the enum as it'll now be used in areas such as the chan closer.

…to accept In this commit, we add a new NewCommitState struct. This preps us for the future change wherein a partial signature is also added to the mix. All related tests and type signatures have also been updated accordingly.

…TLCs

…ation In this commit, we update the channel state machine with a new set of functional options that can be used to create/set the musig session state. When a channel is made during the funding process, the set of nonces we want to use is already known, so we allow them to be passed in. Similarly, once the channel is confirmed, then we'll need to create another channel instance that this times carries the newly generated nonces to send along side funding_locked. We also add some utility methods to permit callers to properly generate nonces in the various contexts.

In this commit, we update the genRemoteHtlcSigJobs function to be able to generate taproot jobs. We also modify the sigpool to now use a input.Signature everywhere. This'll allow us to pass around both ECDSA and Schnorr signatures via the same interface. We use a tapscript sighash in this case, as all the HTLC spends will actually be script path spends.

In this commit, we update the genHtlcSigValidationJobs function to be taproot aware. As we actually need a schnorr signature for the taproot validation, we need to coerce the entire wire type into a schnorr sig with the ForceSchnorr() method.

In this commit, we update the co-op close flow to support the new musig2 keyspend flow. We'll use some new functional options to allow a caller to pass in an active musig2 session. If this is present, then we'll use that to complete the musig2 flow by signing with a partial signature, and then ultimately combining the signatures at the end.

In this commit, we fix a bug in the `deriveMusig2Shachain` function where it didn't actually use the passed in revocation root as part of the hmac invocation. We also modify the function to be more generally useable as well, as now the caller can just pass in the revocation root things should be derived from.

In this commit, we update the ChanSyncMsg to populate nonce information. With this change, we can now hide nonce generation further down in the pipeline and ensure that all callers will have the expected fields populated.

Before this commit, we would conditionally generate nonces in RevokeCurrentCommitment. We move this to generateRevocation as this is called when doing channel sync, and we want to make sure we send the correct set of nonces.

In this commit, we update the logic to handle nonce init in ProcessChanSyncMsg. Once a channel is already open, this is where we'll get the new nonce data from the remote party we'll use to gain the nonce we need to sign for their next state.

Roasbeef added the taproot chans label Jan 20, 2023

Roasbeef mentioned this pull request Jan 20, 2023

[7/?] - funding: update funding manager w/ new musig2+taproot funding flow #7346

Merged

7 tasks

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

saubyk requested a review from Crypt-iQ January 25, 2023 20:05

saubyk assigned Roasbeef Jan 26, 2023

joostjager reviewed Jan 31, 2023

View reviewed changes

lnwallet/channel.go Outdated Show resolved Hide resolved

lnwallet/channel.go Outdated Show resolved Hide resolved

lnwallet/channel.go Show resolved Hide resolved

lnwallet/chancloser/chancloser.go Show resolved Hide resolved

Roasbeef requested a review from ellemouton February 14, 2023 18:35

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

yyforyongyu reviewed Feb 21, 2023

View reviewed changes

ellemouton reviewed Mar 7, 2023

View reviewed changes

saubyk modified the milestones: v0.16.1, v0.17.0 Mar 13, 2023

Roasbeef changed the base branch from master to simple-taproot-funding-reservation April 27, 2023 00:08

Roasbeef force-pushed the simple-taproot-chans-channel-statemachine branch from 9636891 to 717c9a3 Compare April 27, 2023 00:10

Crypt-iQ reviewed May 3, 2023

View reviewed changes

lnwallet/channel.go Show resolved Hide resolved

lnwallet/channel.go Outdated Show resolved Hide resolved

lnwallet/channel.go Outdated Show resolved Hide resolved

Crypt-iQ reviewed May 8, 2023

View reviewed changes

lnwallet/channel.go Outdated Show resolved Hide resolved

Crypt-iQ reviewed May 12, 2023

View reviewed changes

lnwallet/chancloser/chancloser.go Show resolved Hide resolved

Roasbeef force-pushed the simple-taproot-funding-reservation branch from 58e6bef to ce6879c Compare May 12, 2023 20:44

Roasbeef force-pushed the simple-taproot-chans-channel-statemachine branch from 717c9a3 to 4a43a07 Compare May 12, 2023 20:53

yyforyongyu reviewed Jun 12, 2023

View reviewed changes

Roasbeef force-pushed the simple-taproot-funding-reservation branch 2 times, most recently from 781a70f to 2d4df80 Compare June 14, 2023 03:53

Roasbeef force-pushed the simple-taproot-chans-channel-statemachine branch from 4a43a07 to 2882db4 Compare June 14, 2023 03:56

Roasbeef changed the base branch from simple-taproot-funding-reservation to simple-taproot-chans-staging June 14, 2023 03:56

Crypt-iQ reviewed Jun 22, 2023

View reviewed changes

lnwallet/channel.go Show resolved Hide resolved

lnwallet/channel.go Outdated Show resolved Hide resolved

lnwallet/channel.go Show resolved Hide resolved

Roasbeef force-pushed the simple-taproot-chans-channel-statemachine branch 2 times, most recently from b9e8011 to ee8526b Compare June 24, 2023 01:09

Roasbeef force-pushed the simple-taproot-chans-channel-statemachine branch from ee8526b to 763d774 Compare July 3, 2023 01:16

Roasbeef requested a review from ellemouton July 3, 2023 01:16

Roasbeef force-pushed the simple-taproot-chans-channel-statemachine branch from e16b892 to d51ad03 Compare July 14, 2023 02:04

Roasbeef requested review from ellemouton and Crypt-iQ July 14, 2023 02:06

Roasbeef force-pushed the simple-taproot-chans-channel-statemachine branch 2 times, most recently from b3a68b0 to 07d8f36 Compare July 14, 2023 02:22

ellemouton reviewed Jul 18, 2023

View reviewed changes

yyforyongyu reviewed Jul 18, 2023

View reviewed changes

Crypt-iQ approved these changes Jul 19, 2023

View reviewed changes

Roasbeef added 17 commits July 21, 2023 18:28

lnwallet: export MusigCommitType enum

3a94232

We need to export the enum as it'll now be used in areas such as the chan closer.

lnwallet: update CreateHtlcTimeoutTx+CreateHtlcSuccessTx for taproot

f60c547

lnwallet: update genHTLC script to support segwit v0 + v1 (taproot) H…

e97f0c6

…TLCs

lnwallet: update testAddSettleWorkflow to test the new taproot flow

b0cda76

lnwallet/chanvalidate: update ValidateChannel to recognize taproot chans

9d427cd

lnwallet: return structured error from VerifyCommitSig

1790723

input: ensure sessionOpts is properly threaded through

e017fc6

channeldb: update ChanSyncMsg to populate nonce info

ad6bebc

In this commit, we update the ChanSyncMsg to populate nonce information. With this change, we can now hide nonce generation further down in the pipeline and ensure that all callers will have the expected fields populated.

lnwallet: move nonce generation into generateRevocation

40d22d7

Before this commit, we would conditionally generate nonces in RevokeCurrentCommitment. We move this to generateRevocation as this is called when doing channel sync, and we want to make sure we send the correct set of nonces.

lnwallet: refactor revocation tests to test for tweakless+taproot

5c09d61

Roasbeef force-pushed the simple-taproot-chans-channel-statemachine branch from 07d8f36 to 5c09d61 Compare July 21, 2023 17:09

multi: fix linter warnings

7764f65

Roasbeef force-pushed the simple-taproot-chans-channel-statemachine branch from d4b6877 to 7764f65 Compare July 23, 2023 16:00

Roasbeef merged commit 09200ca into lightningnetwork:simple-taproot-chans-staging Jul 23, 2023
20 of 24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[6/?] - lnwallet+chancloser: update channel state machine and co-op close for musig2 flow #7345

[6/?] - lnwallet+chancloser: update channel state machine and co-op close for musig2 flow #7345

Roasbeef commented Jan 20, 2023 •

edited

yyforyongyu Feb 21, 2023

Roasbeef Jun 27, 2023 •

edited

ellemouton left a comment

Roasbeef commented Jul 14, 2023

ellemouton left a comment •

edited

ellemouton Jul 14, 2023

ellemouton Jul 14, 2023

ellemouton Jul 14, 2023

ellemouton Jul 14, 2023

ellemouton Jul 18, 2023

ellemouton Jul 18, 2023 •

edited

Roasbeef Jul 18, 2023

Roasbeef Jul 18, 2023

ellemouton Jul 18, 2023

yyforyongyu left a comment

yyforyongyu Jul 18, 2023

yyforyongyu Jul 18, 2023

yyforyongyu Jul 18, 2023

Crypt-iQ Jul 19, 2023

		@@ -3299,7 +3299,7 @@ func genRemoteHtlcSigJobs(keyRing *CommitmentKeyRing,

		// If the HTLC isn't dust, then we'll create an empty sign job

	// again to broadcast our commitment hte current height will be used.
	// again to broadcast our commitment the current height will be used.

[6/?] - lnwallet+chancloser: update channel state machine and co-op close for musig2 flow #7345

[6/?] - lnwallet+chancloser: update channel state machine and co-op close for musig2 flow #7345

Conversation

Roasbeef commented Jan 20, 2023 • edited

Change Description

Steps to Test

Pull Request Checklist

Testing

Code Style and Documentation

Choose a reason for hiding this comment

Roasbeef Jun 27, 2023 • edited

Choose a reason for hiding this comment

ellemouton left a comment

Choose a reason for hiding this comment

Roasbeef commented Jul 14, 2023

ellemouton left a comment • edited

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

ellemouton Jul 18, 2023 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef commented Jan 20, 2023 •

edited

Roasbeef Jun 27, 2023 •

edited

ellemouton left a comment •

edited

ellemouton Jul 18, 2023 •

edited