multi: support non-interactive collectible sends #176

jharveyb · 2022-10-20T19:58:04Z

Depends on #159, addresses #99 and #121.

Adds support for non-interactive sends of collectibles by allowing splits bearing collectibles, iff they have an unspendable root and exactly one non-root locator.

Fixes #99
Fixes #121

Roasbeef

Set of changes looks pretty minimal which is great, will attempt to dig into the test failure when an asset w/ emission enabled is used to send. Might be some edge case in key family handling in the db, or just the coin selection portion itself.

Roasbeef · 2022-10-25T00:49:57Z

commitment/commitment_test.go

+					t, genesisCollectible,
+					familyKeyCollectible,
+				)
+				root := &SplitLocator{


Roasbeef · 2022-10-25T00:50:38Z

commitment/split.go

@@ -151,6 +150,18 @@ func NewSplitCommitment(input *asset.Asset, outPoint wire.OutPoint,
 		return nil, ErrInvalidSplitLocator
 	}

+	// To transfer a collectible with a split, the split root must be


Roasbeef · 2022-10-25T00:51:52Z

vm/vm.go

-	case vm.newAsset.Type != asset.Normal ||
-		vm.splitAsset.Type != asset.Normal:
-
+	case vm.newAsset.Type != vm.splitAsset.Type:


Love how compact all these changes are, gotta love when a plan nicely falls into place 🤓*

* hidden bugs may apply

Roasbeef · 2022-10-25T00:53:22Z

tarofreighter/chain_porter.go

-	// need a split at all. In this case, we fully consume an input asset,
-	// so the asset created is the same asset w/ the new script key in
-	// place.
-	case SendStatePreparedComplete:


We can remove the enum value definition above as well.

Looks like SendStatePreparedComplete still exists in parcel.go.

itest/collectible_split_test.go

Roasbeef · 2022-10-25T00:56:19Z

itest/collectible_split_test.go

+		err          error
+	)
+
+	for i := 0; i < numSends; i++ {


I think we can somewhat easily (factoring in the optional key family param) abstract this section into a sort of pingPongAsset helper func.

Roasbeef · 2022-10-25T01:04:22Z

Confirmed, that the test works when using a collectible that doesn't support emission.

Running the normal test I run into:


panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x0 pc=0x1023cd454]

goroutine 1339 [running]:
github.com/lightninglabs/taro/mssmt.(*BranchNode).NodeHash(0x0?)
	/Users/roasbeef/gocode/src/github.com/lightninglabs/taro/mssmt/node.go:210 +0x24
github.com/lightninglabs/taro/commitment.(*TaroCommitment).TapLeaf(0x140011f1228)
	/Users/roasbeef/gocode/src/github.com/lightninglabs/taro/commitment/taro.go:156 +0x30
github.com/lightninglabs/taro/commitment.(*TaroCommitment).TapscriptRoot(0x7f17870c4797d810?, 0x0)
	/Users/roasbeef/gocode/src/github.com/lightninglabs/taro/commitment/taro.go:181 +0x2c
github.com/lightninglabs/taro/tarofreighter.(*ChainPorter).stateStep(0x140005bbf80, {0x9, {{0xdb, 0x3}, 0x14000634000}, {0x14000048780, 0x104012a80}, {{{0xa7, 0x74, 0x1, ...}, ...}, ...}, ...})
	/Users/roasbeef/gocode/src/github.com/lightninglabs/taro/tarofreighter/chain_porter.go:884 +0x1914
github.com/lightninglabs/taro/tarofreighter.(*ChainPorter).advanceStateUntil(0x140005bbf80, 0x140010f3ec8, 0xa)
	/Users/roasbeef/gocode/src/github.com/lightninglabs/taro/tarofreighter/chain_porter.go:493 +0x12c
github.com/lightninglabs/taro/tarofreighter.(*ChainPorter).taroPorter(0x140005bbf80)
	/Users/roasbeef/gocode/src/github.com/lightninglabs/taro/tarofreighter/chain_porter.go:226 +0x278
created by github.com/lightninglabs/taro/tarofreighter.(*ChainPorter).Start.func1
	/Users/roasbeef/gocode/src/github.com/lightninglabs/taro/tarofreighter/chain_porter.go:131 +0x284

lightninglabs-deploy · 2022-11-04T22:22:40Z

@Roasbeef: review reminder

Roasbeef

LGTM 💈

Final comment is that I think we can safely drop forcing the family key to be even/odd given that we'll now always serialize the full version.

Roasbeef · 2022-11-08T02:55:05Z

asset/asset.go

-	return tweakedPrivKey.PubKey(), sig, nil
+	// Since we'll never query lnd for a tweaked key, it doesn't matter if
+	// we lose the parity information here.
+	tweakedPubKey, _ := schnorr.ParsePubKey(


I don't think we need this anymore after #187 ?

Roasbeef

LGTM 💈

Final comment is that I think we can safely drop forcing the family key to be even/odd given that we'll now always serialize the full version.

Roasbeef

LGTM 💈

Final comment is that I think we can safely drop forcing the family key to be even/odd given that we'll now always serialize the full version.

Since all spends now use splits, input validation should check for full-value sends, to determine if the split must use a zero-value root.

Simplify the conditions for using an unspendable script key to match the updates to input validation.

guggero

Very nice and clean, LGTM 🎉

guggero · 2022-11-09T18:11:19Z

vm/vm_test.go

@@ -362,6 +362,56 @@ func splitFullValueStateTransition(t *testing.T, validRootLocator,
 	}
 }

+func splitCollectibleStateTransition(t *testing.T,


nit: the outer t isn't needed. But I see that it's there in the other test generator functions too, so non blocking.

guggero · 2022-11-09T18:17:45Z

tarofreighter/chain_porter.go

-	// need a split at all. In this case, we fully consume an input asset,
-	// so the asset created is the same asset w/ the new script key in
-	// place.
-	case SendStatePreparedComplete:


Looks like SendStatePreparedComplete still exists in parcel.go.

guggero · 2022-11-09T18:19:29Z

tarofreighter/chain_porter.go

@@ -600,6 +600,16 @@ func (p *ChainPorter) stateStep(currentPkg sendPackage) (*sendPackage, error) {
 		// to complete the send w/o merging inputs.
 		assetInput := elgigibleCommitments[0]

+		// If the key found for the input UTXO is not from the Taro
+		// keyfamily, something has gone wrong with the DB.
+		if assetInput.InternalKey.Family != tarogarden.TaroKeyFamily {


Did we run into this somewhere? Or what's the reason for this specific check? Mostly just curious.

I think was added during the debugging phase as a sort of sanity check to help track down the bug.

Roasbeef reviewed Oct 25, 2022

View reviewed changes

Roasbeef linked an issue Oct 25, 2022 that may be closed by this pull request

addr+bip-taro-addr: extend address scheme logic to work for collectibles and also full value sends #99

Closed

jharveyb force-pushed the splits-collectibles branch from 18649b2 to ad64cd4 Compare October 25, 2022 18:48

jharveyb mentioned this pull request Nov 3, 2022

multi: encode/decode keys with their parity byte #187

Merged

jharveyb added 5 commits November 4, 2022 15:07

commitment: Add support for collectibles in splits

85d2fea

vm: Add support for collectibles in splits

99120e3

taroscript: Add support for collectibles in splits

ef012b8

tarofreighter: Send collectibles via splits

9c6bbe6

itest: Add new collectible round trip test case

7268ef7

jharveyb force-pushed the splits-collectibles branch from ad64cd4 to 46bd219 Compare November 4, 2022 21:37

jharveyb marked this pull request as ready for review November 4, 2022 21:37

jharveyb requested review from guggero and Roasbeef November 4, 2022 21:37

Roasbeef approved these changes Nov 8, 2022

View reviewed changes

jharveyb added 3 commits November 9, 2022 10:13

tarofreighter: check key family in coin selection

2fe14a8

taroscript: replace needsSplit with fullValue

d209dd5

Since all spends now use splits, input validation should check for full-value sends, to determine if the split must use a zero-value root.

tarofreighter: replace needsSplit with fullValue

929981f

Simplify the conditions for using an unspendable script key to match the updates to input validation.

jharveyb force-pushed the splits-collectibles branch from 46bd219 to 929981f Compare November 9, 2022 15:14

guggero approved these changes Nov 9, 2022

View reviewed changes

Roasbeef merged commit 77b6919 into main Nov 9, 2022

guggero deleted the splits-collectibles branch November 9, 2022 19:30

jharveyb mentioned this pull request Nov 30, 2022

BIP updates #210

Closed

7 tasks

guggero mentioned this pull request Aug 11, 2023

[epic]: BIP updates tracker #293

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multi: support non-interactive collectible sends #176

multi: support non-interactive collectible sends #176

jharveyb commented Oct 20, 2022 •

edited by Roasbeef

Roasbeef left a comment

Roasbeef Oct 25, 2022

Roasbeef Oct 25, 2022

Roasbeef Oct 25, 2022

Roasbeef Oct 25, 2022

guggero Nov 9, 2022

Roasbeef Oct 25, 2022

Roasbeef commented Oct 25, 2022

lightninglabs-deploy commented Nov 4, 2022

Roasbeef left a comment

Roasbeef Nov 8, 2022

Roasbeef left a comment

Roasbeef left a comment

guggero left a comment

guggero Nov 9, 2022

guggero Nov 9, 2022

guggero Nov 9, 2022

Roasbeef Nov 9, 2022

multi: support non-interactive collectible sends #176

multi: support non-interactive collectible sends #176

Conversation

jharveyb commented Oct 20, 2022 • edited by Roasbeef

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

Choose a reason for hiding this comment

Roasbeef commented Oct 25, 2022

lightninglabs-deploy commented Nov 4, 2022

Roasbeef left a 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

Roasbeef left a comment

Choose a reason for hiding this comment

guggero 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

jharveyb commented Oct 20, 2022 •

edited by Roasbeef