Switch account signing over to PSBT, allow deposits/account creation from P2TR inputs #370

guggero · 2022-06-09T12:32:42Z

This PR moves over the signing operations from SignOutputRaw and ComputeInputScript to SignPsbt and FinalizePsbt in order to prepare for Taproot/MuSig2 compatible trader accounts.

With this change we also allow using P2TR wallet inputs for creating new accounts or for depositing into existing accounts.

Pull Request Checklist

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

positiveblue · 2022-06-10T01:17:32Z

rpcserver.go

-			return order.ChannelTypePeerDependent
-		}
-
+		// TODO(guggero): Use order.ChannelTypeScriptEnforced here as


positiveblue · 2022-06-12T16:47:00Z

account/manager.go

-	pIn.FinalScriptWitness, err = serializeWitness([][]byte{ourSig})
+	// We temporarily set the final witness to the partial sig to allow the
+	// extraction of the final TX. Unless we're using the expiry path in
+	// which case we _can_ create the full and final witness.


lightninglabs-deploy · 2022-06-22T12:13:47Z

@Roasbeef: review reminder

Roasbeef

PSBT all the things!

Took an initial pass over the PR, my main question/comment is: why not just also use FundPSBT so we can remove all the coin selection logic (which was copied from lnd mostly so it's duplicated) into a single place? This would also mean that users would be able to use lnd's other coin selection mode (random inputs) as well.

account/manager.go

account/coin_selection.go

account/manager.go

guggero · 2022-06-24T15:39:12Z

@Roasbeef thanks for the review! I did think about using FundPsbt but then was a bit disappointed that we (meaning I) built it in a way that coin selection only happens if you don't specify any inputs. And since we always have the account input, that wouldn't work. But after seeing your trick in Taro where you create a fake output for something to fund the PSBT that got me thinking that we could do some tricks here too. Basically fund a PSBT just for the deposit amount being sent to a P2WSH output and some additional fees (1 extra account witness input), then add the input after funding.
Going to attempt that, will be very nice to get rid of all the coin selection code!

dstadulis · 2022-06-27T15:19:00Z

oli needs to rework to use the PSBT call for the deposit, needs some tricks bc the RPC doesn't allow inputs to specified

guggero · 2022-06-28T20:14:31Z

Addressed all comments but something doesn't seem to be working just yet. Will debug further tomorrow.

Roasbeef

Love all the code deletion in this latest diff! Happy we were able to work around the lack of functionality on the lnd PSBT side. I think we'll need to do something similar on a new project we're working on as well

account/coin_selection.go

Roasbeef · 2022-06-30T01:21:54Z

account/manager.go

+	// Unfortunately the FundPsbt call doesn't allow us to specify _any_
+	// inputs, otherwise it won't perform coin selection at all. So what we
+	// do instead is to fund our account output just for the funding amount
+	// plus whatever we need to pay for the additional input (which we know


Nice work around! We'll probably eventually patch this behavior, but good to be abble to get by w/ less as well.

cc @jharveyb -- this is the workaround I was talking about earlier

Roasbeef · 2022-06-30T01:26:31Z

account/manager.go

-	// We'll start by obtaining our global lock ID.
-	lockID, err := m.cfg.Store.LockID()
+	outputToFund := &wire.TxOut{
+		Value:    int64(depositAmount + acctInputFee),


This should be ok, but part of me wonders if there's some weird edge case here w/ rounding and amounts. Worst case, I guess we get rejected due to having insufficient fees.

Yeah, we should always over estimate by a little bit anyway since most of the time the signatures don't end up being the full 73 bytes. But I went ahead and added a min relay fee check anyway.

guggero · 2022-06-30T12:14:41Z

Okay, after spending way too much time finding out that my problem was actually lightningnetwork/lnd#6626 I created the fix in lightningnetwork/lnd#6687.
Now we have the problem that we can't accept deposits from NP2WKH inputs if you don't have lnd v0.15.1-beta or later.
I added a workaround that just errors out in that case (which is important to do before we contact the auctioneer, otherwise we'll end up with an account diff that never confirms).

I hope that workaround is okay!

With lnd 0.15.0 almost out of the door, it is reasonable for us to ask users to be on the latest 0.14.x version. We use the walletrpc.SignPsbt method in Pool which was added in 0.14.2.

dstadulis · 2022-06-30T15:15:11Z

Done, and ready for review from @positiveblue @Roasbeef #370 https://github.com/lightninglabs/subasta/pull/410

Roasbeef · 2022-06-30T23:51:58Z

I added a workaround that just errors out in that case (which is important to do before we contact the auctioneer, otherwise we'll end up with an account diff that never confirms).

Ahh, makes sense. I think this is OK, as it lets us have the code be uniform vs special casing the np2wkh case. TBH, I don't think it's used all that often these days, since even bech32m usage is on the rise.

Roasbeef

LGTM 🎊

positiveblue

🚀

account/manager.go

guggero requested review from Roasbeef and positiveblue June 9, 2022 12:32

guggero force-pushed the sign-psbt branch from f8efc53 to 10f6468 Compare June 9, 2022 12:36

positiveblue reviewed Jun 10, 2022

View reviewed changes

rpcserver.go

return order.ChannelTypePeerDependent

}

// TODO(guggero): Use order.ChannelTypeScriptEnforced here as

Copy link

Contributor

positiveblue Jun 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

guggero force-pushed the sign-psbt branch 3 times, most recently from 8d8eb53 to 17866e9 Compare June 10, 2022 13:04

dstadulis assigned guggero Jun 12, 2022

positiveblue approved these changes Jun 12, 2022

View reviewed changes

guggero force-pushed the sign-psbt branch from 17866e9 to b2f0a2d Compare June 15, 2022 11:52

This was referenced Jun 16, 2022

Upgrade trader accounts to Taproot/MuSig2 #375

Merged

Update dependencies, improve trace logging #376

Merged

guggero force-pushed the sign-psbt branch from b2f0a2d to 5bdcb4d Compare June 22, 2022 12:17

Roasbeef requested changes Jun 23, 2022

View reviewed changes

guggero force-pushed the sign-psbt branch from 5bdcb4d to 31945d6 Compare June 28, 2022 20:12

guggero force-pushed the sign-psbt branch from 31945d6 to d93d8b2 Compare June 29, 2022 08:45

Roasbeef reviewed Jun 30, 2022

View reviewed changes

guggero added 2 commits June 30, 2022 13:46

mod: bump lnd to final, non-RC version

3507759

multi: add chain params and lnd version to account manager

7571464

guggero force-pushed the sign-psbt branch from d93d8b2 to 64fdb20 Compare June 30, 2022 11:47

guggero requested review from Roasbeef and positiveblue June 30, 2022 12:14

guggero added 3 commits June 30, 2022 15:39

account: use PSBT for account signing

74d2ae4

rpcserver+server: bump minimum required lnd version to v0.14.3-beta

433a3c4

With lnd 0.15.0 almost out of the door, it is reasonable for us to ask users to be on the latest 0.14.x version. We use the walletrpc.SignPsbt method in Pool which was added in 0.14.2.

account: refactor expiry witness path

685a13d

guggero added 2 commits June 30, 2022 16:18

account+poolscript: assert minimum relay fee

232578a

account: assert min lnd version for np2wkh

fd3676f

guggero force-pushed the sign-psbt branch from 64fdb20 to fd3676f Compare June 30, 2022 14:18

Roasbeef approved these changes Jun 30, 2022

View reviewed changes

positiveblue approved these changes Jul 4, 2022

View reviewed changes

positiveblue reviewed Jul 4, 2022

View reviewed changes

account/manager.go Show resolved Hide resolved

guggero merged commit b6da8dd into master Jul 4, 2022

guggero deleted the sign-psbt branch July 4, 2022 09:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch account signing over to PSBT, allow deposits/account creation from P2TR inputs #370

Switch account signing over to PSBT, allow deposits/account creation from P2TR inputs #370

guggero commented Jun 9, 2022 •

edited

Loading

positiveblue Jun 10, 2022

positiveblue Jun 12, 2022

lightninglabs-deploy commented Jun 22, 2022

Roasbeef left a comment

guggero commented Jun 24, 2022

dstadulis commented Jun 27, 2022

guggero commented Jun 28, 2022

Roasbeef left a comment

Roasbeef Jun 30, 2022

Roasbeef Jun 30, 2022

Roasbeef Jun 30, 2022

guggero Jun 30, 2022

guggero commented Jun 30, 2022

dstadulis commented Jun 30, 2022

Roasbeef commented Jun 30, 2022

Roasbeef left a comment

positiveblue left a comment

Switch account signing over to PSBT, allow deposits/account creation from P2TR inputs #370

Switch account signing over to PSBT, allow deposits/account creation from P2TR inputs #370

Conversation

guggero commented Jun 9, 2022 • edited Loading

Pull Request Checklist

positiveblue Jun 10, 2022

Choose a reason for hiding this comment

positiveblue Jun 12, 2022

Choose a reason for hiding this comment

lightninglabs-deploy commented Jun 22, 2022

Roasbeef left a comment

Choose a reason for hiding this comment

guggero commented Jun 24, 2022

dstadulis commented Jun 27, 2022

guggero commented Jun 28, 2022

Roasbeef left a comment

Choose a reason for hiding this comment

Roasbeef Jun 30, 2022

Choose a reason for hiding this comment

Roasbeef Jun 30, 2022

Choose a reason for hiding this comment

Roasbeef Jun 30, 2022

Choose a reason for hiding this comment

guggero Jun 30, 2022

Choose a reason for hiding this comment

guggero commented Jun 30, 2022

dstadulis commented Jun 30, 2022

Roasbeef commented Jun 30, 2022

Roasbeef left a comment

Choose a reason for hiding this comment

positiveblue left a comment

Choose a reason for hiding this comment

guggero commented Jun 9, 2022 •

edited

Loading