Upgrade trader accounts to Taproot/MuSig2 #375

guggero · 2022-06-16T18:24:20Z

Depends on #370.

Pull Request Checklist

LndServices minimum version has been updated if new lnd apis/fields are
used.

positiveblue

First pass done, this looks really good @guggero 👏

poolscript/script.go

account/interfaces.go

account/manager.go

server.go

rpcserver.go

guggero · 2022-07-01T19:13:36Z

I addressed all comments and got almost all itests working on the server side.
This is now officially not WIP anymore 🎉

lightninglabs-deploy · 2022-07-25T21:47:00Z

@positiveblue: review reminder

positiveblue

LGTM 🌮

There is a small comment about how we are storing the new field but nothing blocking.

I would add the version field in multiple rpc calls, like ListAccounts. Also it could be useful to add it to the terminal confirmation message before creating an account. However, I can add those two things later 👍

clientdb/account.go

Roasbeef

Amazing PR!!

Love how we're able to handle all the upgrade logic behind the scenes, hidden away from the user. This change will also make it even cheaper (on chain) to buy/sell leases as well, since the account witness is much smaller.

Completed an initial pass, with the most major comments being some wanted fuzzing/testing of the new witness parsing logic as well as all the new scripts added for taproot.

clientdb/account.go

poolscript/log.go

poolscript/script.go

account/manager.go

order/batch.go

order/batch_storer.go

order/batch.go

guggero · 2022-07-29T14:06:20Z

I addressed all comments except for the additional unit tests. I agree they should be added, but I'm going to work on them next week.

dstadulis · 2022-08-02T16:39:15Z

might need to extend unit test frame work

guggero · 2022-08-03T16:10:52Z

I added a set of unit tests for the new spend paths. And I also ran the fuzzer locally for almost 2 hours and didn't find anything so far.

Roasbeef

LGTM 🐞

poolscript/script_test.go

dstadulis · 2022-08-26T22:06:22Z

⚠️ PR will need to go back to sprint as changes have been made to MuSig2 spec regarding x-only keys

Deliverable outcome:

Modify the implemented MuSig2 spec to incoporate recent changes which will promote ISP

guggero · 2022-08-29T07:21:57Z

warning PR will need to go back to sprint as changes have been made to MuSig2 spec regarding x-only keys

Deliverable outcome:
* [ ]  Modify the implemented MuSig2 spec to incoporate recent changes which will promote ISP

Are we sure we want to wait for that? Updating to the latest spec means:

Implement new API in btcd, merge.
Implement changes to MuSig2 API in lnd, merge, release
Use new API in Pool.

I think it would make more sense to instead add a version parameter to the MuSig2 API when we implement this, so we can upgrade later.

Roasbeef · 2022-08-29T19:49:09Z

PR will need to go back to sprint as changes have been made to MuSig2 spec regarding x-only keys

No, it can proceed as defined. As mentioned, those changes themselves are still in a draft state, with no other implementations currently tracking the change (other than the python ref implementation used to generate the test vectors). Correctness is still retained without the changes. It would matter more in a p2p setting as you need cross implementation compatibility.

Ofc, we could delay things, but then what if we hit another change in serialization somewhere that is "better" but doesn't affect correctness. Would we then do the same and pause to target the newer change? The tradeoff of being one of the first in the ecosystem to utilize these new features is that we sort of need to commit to having the proper versioning to handle future updates.

Roasbeef · 2022-08-29T19:51:59Z

@guggero agreed, iiuc we already have the versioning in the proper places as well. An account version is the most apt, since the serialization changes will change the actual aggregate key, so making the jump a future version would be an actual account version bump.

dstadulis · 2022-08-30T16:13:15Z

Agree that versioning should be incorporated here to facilitate it being upgraded. Appears there was some misinterpretation about the code etc constrained/delayed by MuSig2 spec, which won't be the plan.

With this commit we streamline the signing code somewhat to make it easier to understand. With the new flow the spendPkg struct is no longer needed as we directly pass back the (partially) signed TX.

During the integration tests we might want to shut down a trader client through the admin RPC. If that closes the shutdown channel on a package global interceptor, that means we can't start another trader client during that test. We need to make sure we can pass in an interceptor for each trader instance.

The `.UnsignedTx` of a psbt does not include the related SignatureScripts. Because SignatureScripts are not part of the signed data, the partial signature for a psbt that does not include them is a valid signature. However, the `TxHash` will change between a tx that has SignatureScripts set and one that does not. This behaviour led the auctioneer and the client to record the wrong outpoint when using np2wkh.

guggero requested a review from positiveblue June 17, 2022 10:01

positiveblue reviewed Jun 23, 2022

View reviewed changes

poolscript/script.go Show resolved Hide resolved

account/interfaces.go Show resolved Hide resolved

account/manager.go Outdated Show resolved Hide resolved

account/manager.go Outdated Show resolved Hide resolved

server.go Show resolved Hide resolved

rpcserver.go Show resolved Hide resolved

guggero force-pushed the trader-account-taproot branch 3 times, most recently from dfe03c1 to cb22c9d Compare July 1, 2022 19:11

guggero marked this pull request as ready for review July 1, 2022 19:13

guggero requested a review from Roasbeef July 1, 2022 19:13

positiveblue self-requested a review July 4, 2022 03:00

guggero force-pushed the trader-account-taproot branch 2 times, most recently from 68374a5 to 76acc47 Compare July 5, 2022 20:03

dstadulis added the taro Taro Readiness label Jul 25, 2022

positiveblue approved these changes Jul 27, 2022

View reviewed changes

clientdb/account.go Show resolved Hide resolved

Roasbeef requested changes Jul 28, 2022

View reviewed changes

guggero force-pushed the trader-account-taproot branch 6 times, most recently from dccf0a0 to 007610d Compare July 29, 2022 14:04

guggero force-pushed the trader-account-taproot branch 4 times, most recently from 07acb3d to 543f17d Compare August 3, 2022 15:31

guggero requested a review from Roasbeef August 3, 2022 16:10

Roasbeef approved these changes Aug 4, 2022

View reviewed changes

poolscript/script_test.go Show resolved Hide resolved

positiveblue mentioned this pull request Aug 11, 2022

feature: add support for unannounced and zero conf channels #385

Merged

1 task

positiveblue force-pushed the trader-account-taproot branch 2 times, most recently from 665aeca to be2b1a8 Compare August 23, 2022 07:15

guggero force-pushed the trader-account-taproot branch from be2b1a8 to 0f37262 Compare August 23, 2022 11:54

guggero and others added 18 commits September 12, 2022 17:20

auctioneerrpc: add version and nonces for taproot signing

1ef87fe

account+clientdb: add version to account

7bc48a7

poolscript+account+log: add new Taproot scripts

69d3ad6

multi: add account version to RPCs and manager

a63985d

cmd/pool: add account version to CLI

aa5fdf8

account: use spend witness type everywhere

b6013e3

account: refactor away spend package

bec62ef

With this commit we streamline the signing code somewhat to make it easier to understand. With the new flow the spendPkg struct is no longer needed as we directly pass back the (partially) signed TX.

account: refactor account manager tests

355e057

multi: implement taproot accounts

312e206

multi: add new batch version for account auto-upgrade

a6c18a9

order+rpcserver: use account version in fee estimation

1e7c00a

order: allow account version upgrade during batch

221ea0b

order+rpcserver: add MuSig2 batch signing

9638c8a

multi: calculate locked value based on account version

701b999

cmd: parse and print account version field

28e004b

server: bump required lnd version for Taproot to v0.15.1-beta

a1dab84

guggero force-pushed the trader-account-taproot branch from e333a17 to a1dab84 Compare September 12, 2022 15:20

guggero merged commit a12246c into master Sep 12, 2022

guggero deleted the trader-account-taproot branch September 12, 2022 15:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade trader accounts to Taproot/MuSig2 #375

Upgrade trader accounts to Taproot/MuSig2 #375

guggero commented Jun 16, 2022 •

edited

Loading

positiveblue left a comment

guggero commented Jul 1, 2022

lightninglabs-deploy commented Jul 25, 2022

positiveblue left a comment

Roasbeef left a comment

guggero commented Jul 29, 2022

dstadulis commented Aug 2, 2022 •

edited by guggero

Loading

guggero commented Aug 3, 2022

Roasbeef left a comment

dstadulis commented Aug 26, 2022

guggero commented Aug 29, 2022

Roasbeef commented Aug 29, 2022 •

edited

Loading

Roasbeef commented Aug 29, 2022

dstadulis commented Aug 30, 2022

Upgrade trader accounts to Taproot/MuSig2 #375

Upgrade trader accounts to Taproot/MuSig2 #375

Conversation

guggero commented Jun 16, 2022 • edited Loading

Pull Request Checklist

positiveblue left a comment

Choose a reason for hiding this comment

guggero commented Jul 1, 2022

lightninglabs-deploy commented Jul 25, 2022

positiveblue left a comment

Choose a reason for hiding this comment

Roasbeef left a comment

Choose a reason for hiding this comment

guggero commented Jul 29, 2022

dstadulis commented Aug 2, 2022 • edited by guggero Loading

guggero commented Aug 3, 2022

Roasbeef left a comment

Choose a reason for hiding this comment

dstadulis commented Aug 26, 2022

guggero commented Aug 29, 2022

Roasbeef commented Aug 29, 2022 • edited Loading

Roasbeef commented Aug 29, 2022

dstadulis commented Aug 30, 2022

guggero commented Jun 16, 2022 •

edited

Loading

dstadulis commented Aug 2, 2022 •

edited by guggero

Loading

Roasbeef commented Aug 29, 2022 •

edited

Loading