Add GroupKeyReveal V1

[ffranr]

This PR introduces a new group reveal structure in which the tapscript tree is tweaked instead of the internal key being modified with the asset ID. As a result, an external PSBT signer no longer needs to handle the tweaking of an internal key prior to signing.

Work towards solving: #1226

---

This change is 

[ffranr]

I've added a long doc comment to GroupKeyRevealTapscript. Please review what I've described there. Let me know if you agree/disagree.

[coveralls]

Pull Request Test Coverage Report for Build 12405194991

Details

• 236 of 310 (76.13%) changed or added relevant lines in 4 files are covered.
• 19 unchanged lines in 6 files lost coverage.
• Overall coverage increased (+0.1%) to 40.831%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
proof/records.go	10	11	90.91%
asset/encoding.go	25	36	69.44%
asset/asset.go	200	262	76.34%

Files with Coverage Reduction	New Missed Lines	%
tapgarden/planter.go	2	74.12%
commitment/tap.go	2	83.69%
asset/asset.go	3	80.76%
universe/interface.go	4	51.95%
tapdb/universe.go	4	80.91%
tapgarden/caretaker.go	4	68.5%

Totals
Change from base Build 12379489363:	0.1%
Covered Lines:	26043
Relevant Lines:	63783

---

💛 - Coveralls

[guggero]

Only looked at the Godoc comment of GroupKeyRevealTapscript.

[guggero]

Great work! I think if we can just get this part into v0.5.0 we should be set for future upgrades.

[guggero]

nit: We should mention here that when this is None, from the point of view of the user, the group key doesn't have any scripts (so basically BIP-086). But because we always insert two layers of siblings, it'll never actually be a BIP-086 key (the tapscript root will never be empty).

[ffranr]

customTapscriptRoot is the user provided subtree hash. And finalRoot is the tapscript tree root hash which incorporates the asset ID and the user provided subtree.

finalRoot is never none going forwards because the tree will at least include asset ID.

customTapscriptRoot can be none because there may not be any user provided subtree.

[guggero]

Why do we need to hide these fields behind methods? Any downside to just exporting them directly?

[ffranr]

I think they are used in tests mostly for GroupKeyRevealV0.

[ffranr]

The one thing we need to deal with (probably) is:

func GroupKeyRevealRecord(reveal *asset.GroupKeyReveal) tlv.Record {
	recordSize := func() uint64 {
		// TODO(ffranr): Do we need dispatch on record size? I think
		//  this is used for encoding only. And perhaps only to
		//  determine whether there's anything to encode at all?
		if reveal == nil || *reveal == nil {
			return 0
		}
		r := *reveal
		return uint64(
			btcec.PubKeyBytesLenCompressed + len(r.TapscriptRoot()),
		)
	}
	return tlv.MakeDynamicRecord(
		GroupKeyRevealType, reveal, recordSize,
		asset.GroupKeyRevealEncoder, asset.GroupKeyRevealDecoder,
	)
}

I didn't resolve this TODO.

[ffranr]

The one thing we need to deal with (probably) is:

func GroupKeyRevealRecord(reveal *asset.GroupKeyReveal) tlv.Record {
	recordSize := func() uint64 {
		// TODO(ffranr): Do we need dispatch on record size? I think
		//  this is used for encoding only. And perhaps only to
		//  determine whether there's anything to encode at all?
		if reveal == nil || *reveal == nil {
			return 0
		}
		r := *reveal
		return uint64(
			btcec.PubKeyBytesLenCompressed + len(r.TapscriptRoot()),
		)
	}
	return tlv.MakeDynamicRecord(
		GroupKeyRevealType, reveal, recordSize,
		asset.GroupKeyRevealEncoder, asset.GroupKeyRevealDecoder,
	)
}

I didn't resolve this TODO.

Yes, not sure what to do here.

I think recordSize is only relevant for encoding. The problem is that GroupKeyRevealV1 is encoded as a TLV stream, so I’m not sure how to calculate the size of the encoded byte array. I don’t think it’s just the sum of the field sizes because of the TLV metadata.

[guggero]

Very nice work on this, thanks for the extra push to completion!!!

Approach/Concept ACK. Code looks very good too. But will want to do another pass tomorrow with a fresh mind (but don't expect any significant changes).

[Roasbeef]

I think recordSize is only relevant for encoding. The problem is that GroupKeyRevealV1 is encoded as a TLV stream, so I’m not sure how to calculate the size of the encoded byte array

So that's just used to know what the outer length prefix should be. You can either compute a static value as you did there, or just encode it (without the type and length prefix) to see what the size would be if written as a normal TLV stream.

[Roasbeef]

Excellent work!

Did an initial pass through the diff, at times suggesting changes with in-line code comments. I think the only thing missing here is some tests at the proof.Proof level, to make sure things can be properly encoded/decoded with the new reveal type. We should be able to tack this onto the existing randomized tests and test vector generation.

[Roasbeef]

Stye nit: could be combined above into a switch statement.

[ffranr]

This commit is a code move/copy. I was trying not to make any code changes other than copying to maximise clarity and minimise the review burden. I agree that switch statement would be nice! Perhaps after this I can do some refactoring.

[Roasbeef]

FWIW, for just about all these fields (primitive fields) you can use the new RecordT type, here's an example: https://github.com/lightningnetwork/lnd/blob/8ecef03315f749ae8820b4735978ca2e0552de2e/lnwire/musig2.go#L22-L24

So you can just have a single struct, and declare all the fields as attributes, it would look something like:

type (
   GKRVersionType           = new(tlv.TlvType0)
   GKRInternalKeyType       = new(tlv.TlvType2)
   GKRTapsciptRootType     = new(tlv.TlvType4)
   GKRCustomSubtreeRootType = new(tlv.TlvType7)
)
type GRoupKeyRevealV1 struct {
    Version tlv.RecordT[GKRVersionType, uint32]

    InternalKey tlv.RecordT[GKRInternalKeyType, *btcec.PublicKey]

   TapscriptRoot tlv.RecordT[GKRTapsciptRootType, [sha256.Size]byte]
  
   CustomTreeRoot tlv.Record[GKRCustomSubtreeRootType, [sha256.Size]byte]
}

Then your encode+decode is something like:

func (g *GRoupKeyRevealV1) Encode(w io.Writer) error {
    	stream, err := tlv.NewStream(
		g.Version.Record(), g.InternalKey.Record(), g.TapscriptRoot.Record(),g.CustomTreeRoot.Record(),
	)
	if err != nil {
		return err
	}

	return stream.Encode(w)
}

func (g *GRoupKeyRevealV1) Decode(r io.Reader) error {
  	stream, err := tlv.NewStream(
		g.Version.Record(), g.InternalKey.Record(), g.TapscriptRoot.Record(),g.CustomTreeRoot.Record(),
	)
	if err != nil {
		return err
	}

	return stream.Decode(r)
}

func eGkrV1(w io.Writer, val interface{}, _ *[8]byte) error {
	if v, ok := val.(*GRoupKeyRevealV1); ok {
		return v.Encode(w)
	}

	return tlv.NewTypeForEncodingErr(val, "*GRoupKeyRevealV1")
}

func dGkrV1(r io.Reader, val interface{}, _ *[8]byte, l uint64) error {
	if v, ok := val.(*GRoupKeyRevealV1); ok {
		return v.Decode(r)
	}

	return tlv.NewTypeForDecodingErr(val, "*GRoupKeyRevealV1", l, l)
}

func (g *GRoupKeyRevealV1) Record() tlv.Record {
	sizeFunc := func() uint64 {
		var buf bytes.Buffer
		err := pgEncode(&buf)
		if err != nil {
			panic(err)
		}
		return uint64(len(buf.Bytes()))
	}

	return tlv.MakeDynamicRecord(
		0, p, sizeFunc, eGkrV1, dGkrv1,
	)
}

[Roasbeef]

Then I think we can even add the Record() method to the old interface. So you just call that when makign the stream to handle encode/decode.

[ffranr]

I played around with using tlv.RecordT fields for a while but I couldn't get everything to work correctly.

I got lost in the weeds a bit trying to change all the types and modifying how the fields were used. I got thrown converting customSubtreeRoot fn.Option[chainhash.Hash] into a tlv.OptionalRecordT. Maybe the key was to drop chainhash.Hash in favour of [sha256.Size]byte.

I wouldn't mind giving the tlv.RecordT approach another go, I've managed to use it in RFQ requestWireMsgData, but would you mind if I do that after rc3?

[Roasbeef]

Any reason this isn't just txscript.ControlBlock?

Why wouldn't it be serialized as part of the reveal? It includes the internal key and also the inclusion proof all the way up.

[ffranr]

My thinking here is not to include txscript.ControlBlock in the serialization to avoid redundancy. Breakdown:

• InternalKey: Already serialized separately.
• OutputKeyYIsOdd: Derivable from internal key and tapscript root.
• LeafVersion: Typically constant; serialization might not be necessary?
• InclusionProof: Derivable from the genesis asset ID which should be available separately whenever group key reveal is at hand.

I'm aiming to eliminate data duplication, ensuring that each field serves as a definitive "source of truth." And also reduce serialization size.

I suppose if the default value of LeafVersion changes we could version bump group key reveal? Maybe we should just include it now?

[Roasbeef]

Maybe wrap this in another type of a PartialControl block? This gives the proof up to the custom tapscript root, but a spender will still need to provide the other merkle authentication hashes.

[ffranr]

hmm good point.

I would like to revise this for 0.6. Right now, it's useful for testing (checking the tapscript root) so perhaps it's sufficient as is for rc3?

[Roasbeef]

Ah ok, I see now the decomposition of the reveal struct, and the auxiliary tapscript proof information.

[Roasbeef]

So this is now actually a pointer to an interface?

[ffranr]

If val is asserted to be of type GroupKeyReveal rather than *GroupKeyReveal, and val == nil, the type assertion inside GroupKeyRevealDecoder (e.g., typ, ok := val.(GroupKeyReveal)) will fail. This happens because Go's type assertion mechanism requires both a dynamic type and a value to confirm the type, and a nil value with no dynamic type results in ok == false.

For example:

var gkrDecoded GroupKeyReveal
var scratchBuffDecode [8]byte
err := GroupKeyRevealDecoder(
    &buffer, gkrDecoded, &scratchBuffDecode,
    uint64(buffer.Len()),
)

This will fail because GroupKeyRevealDecoder cannot determine whether val implements the GroupKeyReveal interface when val == nil. Since gkrDecoded is not a pointer, it has no dynamic type in this context, making the assertion invalid.

By contrast, if val is of type *GroupKeyReveal, the dynamic type is always present, even when the value is nil. This allows GroupKeyRevealDecoder to qualify val == nil correctly.

I think there might be a way around this using the reflect package but I haven't gotten it to work yet.

[Roasbeef]

Here's a quick mafs version (may need some tweaking) of this test that uses rapid:

func TestGroupKeyTapscriptEncodeDecode(tt *testing.T) {
	ttt.Parallel()

	rapid.Check(tt, func(t *rapid.T) {
		// Generate random test inputs using rapid generators
		internalKey := *rapid.Custom(func(t *rapid.T) *btcec.PublicKey {
			priv, err := btcec.NewPrivateKey()
			if err != nil {
				t.Fatal(err)
			}
			return priv.PubKey()
		}).Draw(t, "internal_key")

		genesisAssetID := ID(rapid.SliceOfN([]byte{}, 32, 32).Draw(t, "genesis_id"))

		// Randomly decide whether to include a custom script
		hasCustomScript := rapid.Bool().Draw(t, "has_custom_script")

		var customSubtreeRoot fn.Option[chainhash.Hash]
		var customScriptLeaf *txscript.TapLeaf

		if hasCustomScript {
			// Generate random script between 1-100 bytes
			scriptSize := rapid.IntRange(1, 100).Draw(t, "script_size")
			customScript := rapid.SliceOfN([]byte{}, scriptSize, scriptSize).Draw(t, "custom_script")
			
			customScriptLeaf = txscript.NewBaseTapLeaf(customScript)
			customSubtreeRoot = fn.Some(customScriptLeaf.TapHash())
		} else {
			customSubtreeRoot = fn.None[chainhash.Hash]()
		}

		// Create and test GroupKeyReveal
		gkr, err := NewGroupKeyRevealV1(
			internalKey, 
			genesisAssetID,
			customSubtreeRoot,
		).Unpack()
		require.NoError(t, err)

		groupPubKey, err := gkr.GroupPubKey(genesisAssetID)
		require.NoError(t, err)

		// Test encoding/decoding
		var buffer bytes.Buffer
		var scratchBuffEncode [8]byte
		err = GroupKeyRevealEncoder(&buffer, &gkr, &scratchBuffEncode)
		require.NoError(t, err)

		var gkrDecoded GroupKeyReveal
		var scratchBuffDecode [8]byte
		err = GroupKeyRevealDecoder(
			&buffer,
			&gkrDecoded,
			&scratchBuffDecode,
			uint64(buffer.Len()),
		)
		require.NoError(t, err)

		// Prepare for comparison by removing non-encoded fields
		gkrV1, ok := gkr.(*GroupKeyRevealV1)
		require.True(t, ok)
		gkrV1.tapscript.customSubtreeInclusionProof = nil

		// Compare decoded with original
		require.Equal(t, gkrV1, gkrDecoded)

		// Verify decoded group public key
		groupPubKeyDecoded, err := gkrDecoded.GroupPubKey(genesisAssetID)
		require.NoError(t, err)
		require.Equal(t, groupPubKey, groupPubKeyDecoded)

		// Test custom subtree if present
		if customSubtreeRoot.IsSome() && customScriptLeaf != nil {
			gkrDecodedV1, ok := gkrDecoded.(*GroupKeyRevealV1)
			require.True(t, ok)

			ctrlBlock, err := gkrDecodedV1.ScriptSpendControlBlock(
				genesisAssetID,
			).Unpack()
			require.NoError(t, err)

			computedRoot := chainhash.Hash(
				ctrlBlock.RootHash(customScriptLeaf.Script),
			)
			require.Equal(t, gkrDecodedV1.tapscript.root, computedRoot)
		}
	})
}

Could be made more succinct with custom generators.

[ffranr]

Latest commits will include this test 👍 Thanks!

[guggero]

Here's my validation test code that makes sure we can create valid witnesses for group keys with script paths (feel free to add without any attribution):

// TestGroupKeyRevealV1WitnessWithCustomRoot tests the different possible spend
// paths for a group key reveal witness if there are custom scripts.
func TestGroupKeyRevealV1WitnessWithCustomRoot(t *testing.T) {
	var (
		ctx              = context.Background()
		mockKeyRing      = tapgarden.NewMockKeyRing()
		mockSigner       = tapgarden.NewMockGenSigner(mockKeyRing)
		txBuilder        = &tapscript.GroupTxBuilder{}
		txValidator      = &tap.ValidatorV0{}
		hashLockPreimage = []byte("foobar")
	)

	// We expect two keys to be derived from the mock.
	go func() {
		<-mockKeyRing.ReqKeys
		<-mockKeyRing.ReqKeys
	}()

	// The internal key is for the actual internal key of the group.
	internalKeyDesc, err := mockKeyRing.DeriveNextTaprootAssetKey(ctx)
	require.NoError(t, err)

	// The second key is used for a signature spend within a tapscript leaf
	// of the custom tapscript tree.
	secondKeyDesc, err := mockKeyRing.DeriveNextTaprootAssetKey(ctx)
	require.NoError(t, err)

	hashLockLeaf := test.ScriptHashLock(t, hashLockPreimage)
	schnorrSigLeaf := test.ScriptSchnorrSig(t, secondKeyDesc.PubKey)

	userRoot := txscript.AssembleTaprootScriptTree(
		hashLockLeaf, schnorrSigLeaf,
	).RootNode.TapHash()

	spendTestCases := []struct {
		name       string
		genWitness func(*testing.T, *asset.Asset,
			asset.GroupKeyRevealV1) wire.TxWitness
	}{{
		name: "key spend",
		genWitness: func(t *testing.T, a *asset.Asset,
			gkr asset.GroupKeyRevealV1) wire.TxWitness {

			genTx, prevOut, err := txBuilder.BuildGenesisTx(a)
			require.NoError(t, err)

			witness, err := signGroupKeyV1(
				internalKeyDesc, gkr, genTx, prevOut,
				mockSigner, nil,
			)
			require.NoError(t, err)

			return witness
		},
	}, {
		name: "script spend with preimage",
		genWitness: func(t *testing.T, a *asset.Asset,
			gkr asset.GroupKeyRevealV1) wire.TxWitness {

			controlBlock, err := gkr.ScriptSpendControlBlock(
				a.ID(),
			).Unpack()
			require.NoError(t, err)

			controlBlock.InclusionProof = bytes.Join([][]byte{
				fn.ByteSlice(schnorrSigLeaf.TapHash()),
				controlBlock.InclusionProof,
			}, nil)
			controlBlockBytes, err := controlBlock.ToBytes()
			require.NoError(t, err)

			// Witness is just the preimage, the script and the
			// control block.
			return wire.TxWitness{
				hashLockPreimage,
				hashLockLeaf.Script,
				controlBlockBytes,
			}
		},
	}, {
		name: "script spend with signature",
		genWitness: func(t *testing.T, a *asset.Asset,
			gkr asset.GroupKeyRevealV1) wire.TxWitness {

			genTx, prevOut, err := txBuilder.BuildGenesisTx(a)
			require.NoError(t, err)

			controlBlock, err := gkr.ScriptSpendControlBlock(
				a.ID(),
			).Unpack()
			require.NoError(t, err)

			controlBlock.InclusionProof = bytes.Join([][]byte{
				fn.ByteSlice(hashLockLeaf.TapHash()),
				controlBlock.InclusionProof,
			}, nil)
			controlBlockBytes, err := controlBlock.ToBytes()
			require.NoError(t, err)

			leafToSign := &psbt.TaprootTapLeafScript{
				ControlBlock: controlBlockBytes,
				Script:       schnorrSigLeaf.Script,
				LeafVersion:  txscript.BaseLeafVersion,
			}

			witness, err := signGroupKeyV1(
				secondKeyDesc, gkr, genTx, prevOut, mockSigner,
				leafToSign,
			)
			require.NoError(t, err)

			return witness
		},
	}}

	for _, tc := range spendTestCases {
		t.Run(tc.name, func(tt *testing.T) {
			randAsset := asset.RandAsset(tt, asset.Normal)
			genAssetID := randAsset.ID()
			groupKeyReveal, err := asset.NewGroupKeyRevealV1(
				*internalKeyDesc.PubKey, genAssetID,
				fn.Some(userRoot),
			).Unpack()
			require.NoError(tt, err)

			// Set the group key on the asset, since it's a randomly
			// created group key otherwise.
			groupPubKey, err := groupKeyReveal.GroupPubKey(
				genAssetID,
			)
			require.NoError(tt, err)
			randAsset.GroupKey = &asset.GroupKey{
				RawKey:        internalKeyDesc,
				GroupPubKey:   *groupPubKey,
				TapscriptRoot: groupKeyReveal.TapscriptRoot(),
			}
			randAsset.PrevWitnesses = []asset.Witness{
				{
					PrevID: &asset.PrevID{},
				},
			}

			witness := tc.genWitness(tt, randAsset, groupKeyReveal)
			randAsset.PrevWitnesses[0].TxWitness = witness

			err = txValidator.Execute(
				randAsset, nil, nil, proof.MockChainLookup,
			)
			require.NoError(tt, err)
		})
	}
}

// TestGroupKeyRevealV1WitnessNoScripts tests the key spend path for a group key
// reveal witness if there are no custom scripts.
func TestGroupKeyRevealV1WitnessNoScripts(t *testing.T) {
	var (
		ctx         = context.Background()
		mockKeyRing = tapgarden.NewMockKeyRing()
		mockSigner  = tapgarden.NewMockGenSigner(mockKeyRing)
		txBuilder   = &tapscript.GroupTxBuilder{}
		txValidator = &tap.ValidatorV0{}
	)

	// We expect just one key to be derived from the mock.
	go func() {
		<-mockKeyRing.ReqKeys
	}()

	// The internal key is for the actual internal key of the group.
	internalKeyDesc, err := mockKeyRing.DeriveNextTaprootAssetKey(ctx)
	require.NoError(t, err)

	randAsset := asset.RandAsset(t, asset.Normal)
	genAssetID := randAsset.ID()
	groupKeyReveal, err := asset.NewGroupKeyRevealV1(
		*internalKeyDesc.PubKey, genAssetID, fn.None[chainhash.Hash](),
	).Unpack()
	require.NoError(t, err)

	// Set the group key on the asset, since it's a randomly created group
	// key otherwise.
	groupPubKey, err := groupKeyReveal.GroupPubKey(genAssetID)
	require.NoError(t, err)
	randAsset.GroupKey = &asset.GroupKey{
		RawKey:        internalKeyDesc,
		GroupPubKey:   *groupPubKey,
		TapscriptRoot: groupKeyReveal.TapscriptRoot(),
	}
	randAsset.PrevWitnesses = []asset.Witness{
		{
			PrevID: &asset.PrevID{},
		},
	}

	genTx, prevOut, err := txBuilder.BuildGenesisTx(randAsset)
	require.NoError(t, err)

	witness, err := signGroupKeyV1(
		internalKeyDesc, groupKeyReveal, genTx, prevOut, mockSigner,
		nil,
	)
	require.NoError(t, err)

	randAsset.PrevWitnesses[0].TxWitness = witness

	err = txValidator.Execute(
		randAsset, nil, nil, proof.MockChainLookup,
	)
	require.NoError(t, err)
}

// signGroupKeyV1 is the equivalent for asset.DeriveGroupKey but for a V1 key.
func signGroupKeyV1(keyDesc keychain.KeyDescriptor, gk asset.GroupKeyRevealV1,
	genTx *wire.MsgTx, prevOut *wire.TxOut, signer asset.GenesisSigner,
	leafToSign *psbt.TaprootTapLeafScript) (wire.TxWitness, error) {

	signDesc := &lndclient.SignDescriptor{
		KeyDesc:    keyDesc,
		TapTweak:   gk.TapscriptRoot(),
		Output:     prevOut,
		HashType:   txscript.SigHashDefault,
		InputIndex: 0,
		SignMethod: input.TaprootKeySpendSignMethod,
	}

	if leafToSign != nil {
		signDesc.SignMethod = input.TaprootScriptSpendSignMethod
		signDesc.WitnessScript = leafToSign.Script
	}

	sig, err := signer.SignVirtualTx(signDesc, genTx, prevOut)
	if err != nil {
		return nil, err
	}

	witness := wire.TxWitness{sig.Serialize()}

	// If this was a script spend, we also have to add the script itself and
	// the control block to the witness, otherwise the verifier will reject
	// the generated witness.
	if signDesc.SignMethod == input.TaprootScriptSpendSignMethod &&
		leafToSign != nil {

		witness = append(
			witness, signDesc.WitnessScript,
			leafToSign.ControlBlock,
		)
	}

	return witness, nil
}

Must be added to tapgarden/planter_test.go to work.
A lot of that code will then be useful later on when we implement the actual signing changes in the planter.

[ffranr]

Must be added to tapgarden/planter_test.go to work.

Works great, thank you!

[ffranr]

I've added a small unit test which covers Proof encoding/decoding with GroupKeyRevealV1.

I think I've addressed all review comments.

[Roasbeef]

Reviewed 2 of 3 files at r1, 3 of 3 files at r2, 1 of 1 files at r3, 1 of 2 files at r5, 2 of 4 files at r6, 3 of 3 files at r7, 2 of 2 files at r8, all commit messages.
Reviewable status: all files reviewed, 34 unresolved discussions (waiting on @ffranr and @guggero)

[Roasbeef]

LGTM 🧞‍♂️

[guggero]

Awesome work, LGTM 🎉