psbt script spend [3/3]: allow signing for asset script path spends through PSBTs #245

guggero · 2023-01-25T17:03:19Z

~~Depends on #243.~~
Depends on #244.
~~Depends on btcsuite/btcd#1942.~~

Fixes #75.

This PR contains the PSBT serialization logic for transporting asset related meta data in a BIP-174 based PSBT packet as well as all the required RPCs to allow creating and sending to a Taro address that locks assets to a script spend path.

jharveyb

Looking good! Surprisingly small changes to get script working. Only major suggested change is the FetchCommitment usage.

Will revisit to double-check the encode and Taproot logic.

itest/psbt_test.go

jharveyb · 2023-02-01T18:44:05Z

itest/psbt_test.go

+		bobScriptKeyDesc.RawKeyBytes,
+	)
+	require.NoError(t.t, err)
+	bobInternalKeyDesc, err := bobLnd.WalletKitClient.DeriveNextKey(


General comment: Wondering if this is an RPC we should expose from tarod in the future? May be relevant for tracking which keys to backup as well.

As in re-expose the lnd API used to create keys in the first place?

Yeah, I think it could be useful to have this call be threaded through the daemon so we don't need to know what key family to use in lnd when talking to tarod. Will add that to the PR that adds the BTC level PSBT stuff (for funding and signing the anchor transactions externally).

itest/psbt_test.go

tarodb/assets_store.go

taropsbt/decode.go

taropsbt/interface.go

taroscript/tx.go

tarofreighter/chain_porter.go

Roasbeef

Excellent work! Gotta love when a few well crafted abstractions nicely fall into place.

See comments in line, some carried over from the PR before it was split up.

Few things popped out:

Ensuring that all the decoding + parsing is generic up until we restrict things to a single input deeper within the pipeline.
Adding a similar coin selection mutex like we have in lnd over everything, which ensures that we don't have weird double spending issues. Alternatively, we can pipe everything through the porter and have it do single threaded coin selection + locking accounting.
Update the test to also have a checksig in addition to the pre-image reveal so the vPSBT signing can be exercised there.

taropsbt/decode.go

Roasbeef · 2023-02-02T01:37:27Z

tarodb/assets_store.go

+
+	if groupKey != nil {
+		key := groupKey.GroupPubKey
+		filter.KeyGroupFilter = key.SerializeCompressed()


isn't the asset ID enough to be able to uniquely pick out an asset?

No, in case of a normal asset where I have multiple coins, we actually need the script key, which I added now. We don't necessarily need the group key for our specific query, but because this function creates a generic filter, I'm leaving that in for now.

tarodb/assets_store.go

Roasbeef · 2023-02-02T01:47:45Z

rpcserver.go

+	rpcsLog.Debugf("Selected commitment for anchor point %v, requesting "+
+		"delivery", inputCommitment.AnchorPoint)
+
+	resp, err := r.cfg.ChainPorter.RequestSignedDelivery(


So then this also assumes that the wallet as is will fully handle all the PSBT components? Could imagine another world where there's another external round to sign the Bitcoin portion (as that has a multi-sig/musig2 layer or w/e).

Yes, definitely. The BTC level PSBT components will come in a later PR and give you full control over how the anchor TX is created, signed and published.

tarorpc/assetwalletrpc/assetwallet.proto

itest/psbt_test.go

Roasbeef · 2023-02-02T01:50:54Z

itest/psbt_test.go

+		bobScriptKeyDesc.RawKeyBytes,
+	)
+	require.NoError(t.t, err)
+	bobInternalKeyDesc, err := bobLnd.WalletKitClient.DeriveNextKey(


As in re-expose the lnd API used to create keys in the first place?

itest/psbt_test.go

dstadulis · 2023-02-06T16:36:46Z

full value send test needs to be written.

guggero · 2023-02-09T16:51:51Z

Thanks a lot for all the feedback, @Roasbeef and @jharveyb!

I addressed all of the comments and rebased on the latest version of part 2.

* Ensuring that all the decoding + parsing is generic up until we restrict things to a single input deeper within the pipeline.

Good idea, done!

* Adding a similar coin selection mutex like we have in `lnd` over everything, which ensures that we don't have weird double spending issues. Alternatively, we can pipe everything through the porter and have it do single threaded coin selection + locking accounting.

I agree. Though I think a mutex only adds any value if we can also mark a coin as being used/locked in the DB. I created an issue (#261) to track that.

* Update the test to also have a checksig in addition to the pre-image reveal so the vPSBT signing can be exercised there.

Yes, done.

I'm also going to split off the interactive send changes into a separate PR (I guess it will be [4/3] 😅 ), since the changes were a bit more involved than expected.

jharveyb

Looks good, only nits left IMO + changes in preceding PR.

itest/psbt_test.go

tarodb/assets_store_test.go

tarogarden/mock.go

taropsbt/decode.go

taropsbt/encode.go

taroscript/send.go

tarofreighter/parcel.go

guggero · 2023-02-16T13:56:40Z

Addressed all comments and added the input proof validation as well.

jharveyb

Looking good! Just have to remove proof{Encoder,Decoder}.

jharveyb · 2023-02-17T01:10:00Z

tarofreighter/wallet.go

+		// Before we sign the transaction, we want to make sure the
+		// inclusion proof is valid and the asset is actually committed
+		// in the anchor transaction.
+		err := verifyInclusionProof(vPkt.Inputs[idx])


Probably want a TODO here to add a call to ChainBridge later on? To verify the anchor TX from the proof is in the chain.

Great point! That also reminded me that we didn't check the anchor TX in the input vs. the one in the proof, which I added now. So only the "is the block in the chain" logic remains, for which I added a TODO.

tarofreighter/wallet.go

Roasbeef

LGTM 🌳

Just have one clarifying question (about the leaf hashes in the PSBT), and another re a field that doesn't need to be set for a script path spend (the tap tweak).

Roasbeef · 2023-02-22T01:43:51Z

taroscript/tx.go

+		}
+
+		spendDesc.SignMethod = input.TaprootScriptSpendSignMethod
+		spendDesc.TapTweak = vIn.TaprootMerkleRoot


Why is the tap tweak needed for a script spend from the tree? This is only needed for keyspend paths.

Good point, removed.

Roasbeef · 2023-02-22T01:44:25Z

taroscript/tx.go

+	// One leaf hash and a merkle root means we're signing a specific
+	// script.
+	case len(vIn.TaprootMerkleRoot) == sha256.Size &&
+		len(derivation.LeafHashes) == 1:


Does this restrict to only a control block of size 1? So you only have a single sibling.

No, we just only support signing for a single leaf at a time. The PSBT spec allows you to specify multiple leaf hashes and that would indicate to the signer that it should create a signature for each of them. But since we always know which path we're going to use, we haven't implemented this in lnd either. I updated the comment to make it more clear.

To avoid yet another circular dependency, we move the commitment proof related encoders and records into the commitment package.

The asset minting test is currently flaky because the total sum of all asset amounts minted exceeds the database's precision. This should be fixed by building the sum outside of the database. Or by disallowing amounts exceeding a certain number. For now we just fix the flake by choosing smaller random amounts.

This fixes an issue with the itest that the requests were modified in place, affecting the next test that's being run. By copying the request before modifying it, this can be avoided.

guggero requested review from Roasbeef and jharveyb January 25, 2023 17:03

guggero changed the title ~~psbt script spend [2/3]: allow signing for asset script path spends through PSBTs~~ psbt script spend [3/3]: allow signing for asset script path spends through PSBTs Jan 25, 2023

guggero mentioned this pull request Jan 25, 2023

very early preview: refactor send logic towards PSBT based signing #217

Closed

guggero force-pushed the psbt-3-of-3-script-sign branch 2 times, most recently from cc9aed4 to 8afc879 Compare January 30, 2023 10:56

jharveyb reviewed Feb 1, 2023

View reviewed changes

Roasbeef requested changes Feb 2, 2023

View reviewed changes

guggero force-pushed the psbt-3-of-3-script-sign branch 2 times, most recently from 6773c33 to 57b4bf7 Compare February 3, 2023 20:31

guggero force-pushed the psbt-3-of-3-script-sign branch 3 times, most recently from 3a1bfa7 to 16dcf55 Compare February 9, 2023 16:50

guggero requested review from jharveyb and Roasbeef February 9, 2023 16:51

guggero force-pushed the psbt-3-of-3-script-sign branch 2 times, most recently from 8b32e68 to 1b0a54a Compare February 14, 2023 18:15

jharveyb reviewed Feb 14, 2023

View reviewed changes

guggero force-pushed the psbt-3-of-3-script-sign branch from 1b0a54a to 3067bd9 Compare February 16, 2023 13:56

guggero requested a review from jharveyb February 16, 2023 13:56

guggero force-pushed the psbt-3-of-3-script-sign branch from 3067bd9 to 08a32c3 Compare February 16, 2023 15:29

jharveyb approved these changes Feb 17, 2023

View reviewed changes

guggero force-pushed the psbt-3-of-3-script-sign branch 2 times, most recently from 535e7b1 to 3ad1f00 Compare February 20, 2023 22:31

Roasbeef approved these changes Feb 22, 2023

View reviewed changes

guggero added 2 commits February 22, 2023 09:50

commitment+proof: move asset proof encoders/records

be494ed

To avoid yet another circular dependency, we move the commitment proof related encoders and records into the commitment package.

mod+taropsbt: add encoding and decoding of virtual packet

79cd764

guggero added 8 commits February 22, 2023 09:50

tarodb: add FetchCommitment function

36679e2

taroscript: allow custom signing of virtual TX

435ed15

tarofreighter: allow delivery of pre-signed pkt

c815554

multi: add AssetWallet RPC service

197ae69

proof+tarofreighter: validate input proof

358f10e

itest: add psbt itest

3ec35c6

itest: create copy before modifying mint request

f9f6d94

This fixes an issue with the itest that the requests were modified in place, affecting the next test that's being run. By copying the request before modifying it, this can be avoided.

guggero force-pushed the psbt-3-of-3-script-sign branch from 3ad1f00 to 24d4368 Compare February 22, 2023 08:50

fixup! taroscript: allow custom signing of virtual TX

24d4368

guggero added this pull request to the merge queue Feb 22, 2023

Merged via the queue into main with commit 079bc59 Feb 22, 2023

guggero deleted the psbt-3-of-3-script-sign branch February 22, 2023 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

psbt script spend [3/3]: allow signing for asset script path spends through PSBTs #245

psbt script spend [3/3]: allow signing for asset script path spends through PSBTs #245

guggero commented Jan 25, 2023 •

edited

Loading

jharveyb left a comment

jharveyb Feb 1, 2023

Roasbeef Feb 2, 2023

guggero Feb 9, 2023

Roasbeef left a comment

Roasbeef Feb 2, 2023

guggero Feb 3, 2023

Roasbeef Feb 2, 2023

guggero Feb 3, 2023

Roasbeef Feb 2, 2023

dstadulis commented Feb 6, 2023

guggero commented Feb 9, 2023

jharveyb left a comment

guggero commented Feb 16, 2023

jharveyb left a comment

jharveyb Feb 17, 2023

guggero Feb 17, 2023

Roasbeef left a comment

Roasbeef Feb 22, 2023

guggero Feb 22, 2023

Roasbeef Feb 22, 2023

guggero Feb 22, 2023

psbt script spend [3/3]: allow signing for asset script path spends through PSBTs #245

psbt script spend [3/3]: allow signing for asset script path spends through PSBTs #245

Conversation

guggero commented Jan 25, 2023 • edited Loading

jharveyb 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

Roasbeef 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

Choose a reason for hiding this comment

dstadulis commented Feb 6, 2023

guggero commented Feb 9, 2023

jharveyb left a comment

Choose a reason for hiding this comment

guggero commented Feb 16, 2023

jharveyb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef 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

guggero commented Jan 25, 2023 •

edited

Loading