[4/?] - input+lnwallet: prepare input package for funding logic, add new MusigSession abstraction by Roasbeef · Pull Request #7340 · lightningnetwork/lnd

Roasbeef · 2023-01-19T04:15:09Z

Change Description

Depends on #7333 and #7331.

Only the last 3 commits are new. We'll also want to synchronize with #6974 as both the PRs update the deps to use the latest version of musig2.

In this PR, we first do a small refactoring to extract the musig2 session logic into a new package + struct. This allows use to re-use it later in tests w/o creating the entire wallet system.

Building on this, we then do some small prep to update the chanfunding package to be ready for the new musig2 channels.

The final change in this PR adds a new struct to encapsulate the new musig2 session logic for channels. A session can be created as soon as the verification nonce for the local party is known. At any time, we'll have two sessions: one for the remote commitment and one for the local commitment. A session can be finalized once the signing nonce (sent by the remote party) is known. When generating a signature, we'll use the remote party's verification nonce (the remote session), then generate a fresh nonce to complete the session. Similarly, when accepting a new state, we'll use the remote party's signing nonce to finalize the session, then verify the incoming signature.

For our local session, we'll use a counter-based system to generate the nonces we send to the remote party. The underlying counter here is the commitment height, which is then used by the existing shachain producer to generate fresh, but deterministic randomness. This lets us not have to store the secret nonce to disk, instead we'll just re-generate the nonce and use the sig we have on disk to combine, then create the final witness to broadcast.

The next PR in this series uses this PR, and its dependents to implement the funding logic within the wallet itself (reservations).

Steps to Test

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.

ellemouton

First pass looks good! In my next pass I will spend more time understanding the details of the last commit

yyforyongyu · 2023-02-17T12:42:38Z

nit: format

Gotta stamp something in the formatting guide here...

Crypt-iQ

Pretty straightforward, will have to see how it fits in with the other changes

yyforyongyu

I think we are missing unit tests here, can help with it if needed.

lightninglabs-deploy · 2023-05-31T15:04:23Z

@Roasbeef, remember to re-request review from reviewers when ready

Roasbeef · 2023-06-02T01:33:35Z

Initial set of review comment addressed. Will request review again after I put up the unit tests.

Roasbeef · 2023-06-03T01:46:35Z

Unit tests added, PTAL!

yyforyongyu · 2023-06-06T12:32:41Z

+	nonceOpts := []musig2.NonceGenOption{
+		musig2.WithPublicKey(m.localKey.PubKey),
+	}
+	if opts.customRand != nil {


I think we should enforce the check to make sure nonce is not repeated?

Hmm, how would we go about that? The customRand func in this case will use the new shachain root to derive nonces, also we'll start to pass in the txhash being signed into the nonce gen to further bind it.

We only set the custom rand for our local commitment, which means we only sign with this nonce when we go to broadcast.

Here's the argument I posted elsewhere for why imo the current approach is sound:

we only use that verification nonce to sign when we broadcast

we only ever have a single transaction valid to broadcast

after we sign and broadcast, we have that saved tx to rebroadcast, this tx never changes and we can't modify it (since we have another sig)

leaking the nonce needs us to sign with that key w/ two diff messages, which itself would mean we're broadcasting a revoked state

What I meant was, we'd make sure the opts is always set for m.commitType == localCommit, so maybe we can return an error if opts.customRand == nil && m.commitType == localCommit.

By design it's meant to be an optional value though, we maintain similar control flow, but then allow the caller to specify this. I think a better area to add additional protections would be the channel state machine itself (part 6 in this series) where this is used.

This would also break the current test that exercises the "normal" random nonce paths.

ellemouton

I think this is close!

yyforyongyu

This is good to go on my end once the redundant MuSig2Cleanup is handled.

yyforyongyu · 2023-06-08T13:42:01Z

+	nonceOpts := []musig2.NonceGenOption{
+		musig2.WithPublicKey(m.localKey.PubKey),
+	}
+	if opts.customRand != nil {


What I meant was, we'd make sure the opts is always set for m.commitType == localCommit, so maybe we can return an error if opts.customRand == nil && m.commitType == localCommit.

yyforyongyu · 2023-06-08T13:47:36Z

I think MuSig2CombineSig will clean up the session so this will always return an error because the session is not found. Maybe we shouldn't do the auto cleanup in MuSig2CombineSig, or mention it in the method name or docs since it's error-prone.

Ah yeah good point, removed since it isn't necessary.

Initially I wanted to try to clean up the session as soon as possible, but realized that you need the session state present in order to do combine (so you can't clean up, then try to combine later the state won't be found). What I think we should do is add a defer CleanUp() in the link itself, so this way whenever we hang up the connection we remove the active session. We then also want to do this in the state machine after a round trip is complete.

I need to revisit the force close code in the final part of this PR, I think it ends up re-creating the session so it can properly force close.

ellemouton

LGTM pending @yyforyongyu 's comment re the session clean up 🥕 ⚡

(some linter things remain that I assume will be addressed in a final clean up commit once all the others are also in)

In this commit, we extract the musig2 session management into a new module. This allows us to re-use the session logic elsewhere in unit tests so we don't need to instantiate the entire wallet.

In this commit, we update the set of intents and assemblers to recognize musig2. For this change, we use a new bool, `musig2`, then use that to determine if we need to use the new taproot funding scripts or not.

…tment state In this commit, we add a series of abstractions that'll allow us to easily do funding and also state updates for the new taproot channels. A partial session is defined by the knowledge of a verification nonce. Once the remote party sends a signature, we learn of their signing nonce, and can then complete a session. By using a JIT nonce approach, we ensure that the signer can generate their nonces randomly and also at the very last step to avoid having to maintain state. For our local nonces, we also have an option to use a counter based nonce derived from the shachain instead of fully random nonces. This allows us to not have to store ay additional state. Instead, when we need to go to broadcast, we can just regenerate the nonce then use that to broadcast.

In this commit, we eliminate some code duplication by removing the old `HashMutex` struct as it just duplicates all the code with a different type (uint64 and hash). We then make the main Mutex struct take a type param, so the key can be parametrized when the struct is instantiated.

By using the multimutex here, we'll no longer rely on a single mutex for the entire musig session set like we used to. Instead, we can use the session ID to key into a map of mutexes and use those directly.

Roasbeef · 2023-06-09T00:37:20Z

(some linter things remain that I assume will be addressed in a final clean up commit once all the others are also in

Really been fighting the linter on this one...should be ✅ now with the latest push.

yyforyongyu

Really been fighting the linter on this one...

Attack on linter series𖠆

LGTM🎉🎉🎉

Roasbeef added the taproot chans label Jan 19, 2023

Roasbeef mentioned this pull request Jan 20, 2023

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

Merged

7 tasks

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

saubyk requested review from ellemouton and yyforyongyu January 25, 2023 20:04

ellemouton reviewed Jan 26, 2023

View reviewed changes

Comment thread lnwallet/musession/manager.go Outdated

Comment thread lnwallet/chanfunding/assembler.go Outdated

Comment thread lnwallet/chanfunding/canned_assembler.go Outdated

saubyk assigned Roasbeef Jan 26, 2023

joostjager reviewed Jan 31, 2023

View reviewed changes

Comment thread lnwallet/musession/manager.go Outdated

Comment thread lnwallet/chanfunding/canned_assembler.go Outdated

Comment thread lnwallet/musig_session.go Outdated

Comment thread lnwallet/musig_session.go Outdated

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

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

yyforyongyu reviewed Feb 20, 2023

View reviewed changes

ellemouton reviewed Mar 1, 2023

View reviewed changes

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

Crypt-iQ reviewed Apr 26, 2023

View reviewed changes

Comment thread lnwallet/chanfunding/canned_assembler.go Outdated

Roasbeef changed the base branch from master to simple-taproot-chans-scripts April 26, 2023 23:55

Roasbeef force-pushed the simple-taproot-funding branch from 73baf9c to 8b3a3d6 Compare April 27, 2023 00:01

ellemouton reviewed Apr 27, 2023

View reviewed changes

Comment thread input/test_utils.go

Comment thread lnwallet/musession/manager.go Outdated

Comment thread lnwallet/musig_session.go Outdated

Comment thread lnwallet/musig_session.go Outdated

yyforyongyu reviewed May 10, 2023

View reviewed changes

Roasbeef force-pushed the simple-taproot-chans-scripts branch from ff278d8 to d7ccdef Compare May 12, 2023 04:51

Roasbeef force-pushed the simple-taproot-funding branch from 8b3a3d6 to 6fc2387 Compare May 12, 2023 20:40

Roasbeef changed the base branch from simple-taproot-chans-scripts to simple-taproot-chans-staging June 2, 2023 00:25

Roasbeef force-pushed the simple-taproot-funding branch from 6fc2387 to 8819f71 Compare June 2, 2023 00:26

Roasbeef commented Jun 2, 2023

View reviewed changes

Comment thread lnwallet/test_utils.go Outdated

Roasbeef force-pushed the simple-taproot-funding branch 3 times, most recently from 2be6b53 to eecba46 Compare June 2, 2023 23:37

Roasbeef force-pushed the simple-taproot-funding branch 3 times, most recently from 083af6f to a8e8d06 Compare June 3, 2023 00:17

Roasbeef requested review from Crypt-iQ, ellemouton and yyforyongyu June 3, 2023 01:46

Roasbeef force-pushed the simple-taproot-funding branch from a8e8d06 to 947096b Compare June 3, 2023 01:46

yyforyongyu reviewed Jun 6, 2023

View reviewed changes

ellemouton reviewed Jun 6, 2023

View reviewed changes

Comment thread lnwallet/chanfunding/canned_assembler.go Outdated

Comment thread lnwallet/musig_session.go Outdated

Comment thread lnwallet/musig_session.go Outdated

ellemouton self-requested a review June 6, 2023 16:47

ellemouton reviewed Jun 7, 2023

View reviewed changes

Roasbeef force-pushed the simple-taproot-funding branch 2 times, most recently from 691ecd9 to d22a0f9 Compare June 8, 2023 00:07

yyforyongyu reviewed Jun 8, 2023

View reviewed changes

yyforyongyu added the no-changelog label Jun 8, 2023

ellemouton approved these changes Jun 8, 2023

View reviewed changes

Comment thread lnwallet/musig_session.go Outdated

Roasbeef added 6 commits June 8, 2023 17:16

input+wallet: extract musig2 session management into new module

a2f7350

In this commit, we extract the musig2 session management into a new module. This allows us to re-use the session logic elsewhere in unit tests so we don't need to instantiate the entire wallet.

lnwallet/chanfunding: update assemblers to support musig2

02a0671

In this commit, we update the set of intents and assemblers to recognize musig2. For this change, we use a new bool, `musig2`, then use that to determine if we need to use the new taproot funding scripts or not.

input: use multmutex to increase concurrency for musig session manager

0bc8f3f

By using the multimutex here, we'll no longer rely on a single mutex for the entire musig session set like we used to. Instead, we can use the session ID to key into a map of mutexes and use those directly.

lnwallet: add initial unit test coverage for musig chan session

7c85f02

Roasbeef force-pushed the simple-taproot-funding branch from d22a0f9 to 7c85f02 Compare June 9, 2023 00:20

yyforyongyu approved these changes Jun 9, 2023

View reviewed changes

Roasbeef merged commit a8dff96 into lightningnetwork:simple-taproot-chans-staging Jun 10, 2023

Conversation

Roasbeef commented Jan 19, 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!

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!

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Crypt-iQ left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

lightninglabs-deploy commented May 31, 2023

Uh oh!

Roasbeef commented Jun 2, 2023

Uh oh!

Uh oh!

Roasbeef commented Jun 3, 2023

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!

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Roasbeef commented Jan 19, 2023 •

edited

Loading

ellemouton left a comment •

edited

Loading