Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vPSBT: add sighash support (flattened virtual transactions) #759

Closed
wants to merge 2 commits into from
Closed

vPSBT: add sighash support (flattened virtual transactions) #759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1135c4e
tapscript+asset: flatten out virtual transaction
GeorgeTsagk Jan 9, 2024
ca7b115
vm: use new virtual transaction interface
GeorgeTsagk Jan 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions asset/asset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,8 @@ const (
// output key, allowing the ability for an asset to indirectly commit to
// multiple spending conditions.
ScriptV0 ScriptVersion = 0

ScriptV1 ScriptVersion = 1
)

// AssetGroup holds information about an asset group, including the genesis
Expand Down
7 changes: 5 additions & 2 deletions asset/mock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ func virtualGenesisTxOut(newAsset *Asset) (*wire.TxOut, error) {
// MockGroupTxBuilder.
func virtualGenesisTx(newAsset *Asset) (*wire.MsgTx, error) {
var (
txIn *wire.TxIn
txIn []*wire.TxIn
Copy link
Member

Choose a reason for hiding this comment

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

nit: rename to txIns to not shadow the name in the loop below (for _, txIn := range txIn)?

err error
)

Expand All @@ -159,7 +159,10 @@ func virtualGenesisTx(newAsset *Asset) (*wire.MsgTx, error) {
// With our single input and output mapped, we're ready to construct our
// virtual transaction.
virtualTx := wire.NewMsgTx(2)
virtualTx.AddTxIn(txIn)

Copy link
Member

Choose a reason for hiding this comment

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

So this can't be done unconditionally. We need to make a new TAP VM version, then flatten the virtual transaction only for that version.

for _, txIn := range txIn {
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, coming back to this, I think I'm still not totally convinced we need to do this. Input level conditional signing can happen on the Bitcoin layer, in that you use the normal sig hash flags there to bind your Bitcoin level signature to the presence of one or all inputs.

In TAP, we have a sort of output centric model: you validate the output (asset leaf TLV), and the witness data is also stored in the output. You can control the environment needed to validate that output based on the logical spend group. The set of inputs here can only ever be the set of inputs referenced in the witness data for the output. Each input needs a witness, and is effectively always consolidating to that single output (ignoring splits for a second).

With the framing above, in what case would we ever need to sign only some of the inputs in that spend group? Changing the set of inputs actually changes the output, so those need to be fully bound. We don't have the ability to have loose binding of the inputs.

If you consider splits, then that's an area that I think we can already have covered as the sighash flag can determine which of the split out puts are inserted into the split output tree. For all, do everything. For none, do nothing. You can make single work somewhat

With all that said, I think it makes sense to make this an option, if we have indeed found a flow that can't work w/o a flattened virtual txn. Why wouldn't we also flatten the tx out as well?

As mentioned above, this requires updates elsewhere.

virtualTx.AddTxIn(txIn)
}
virtualTx.AddTxOut(txOut)
return virtualTx, nil
}
Expand Down
14 changes: 9 additions & 5 deletions asset/tx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,16 +59,16 @@ func VirtualTxWithInput(virtualTx *wire.MsgTx, input *Asset,

txCopy := virtualTx.Copy()
txCopy.LockTime = uint32(input.LockTime)
txCopy.TxIn[zeroIndex].PreviousOutPoint.Index = idx
txCopy.TxIn[zeroIndex].Sequence = uint32(input.RelativeLockTime)
txCopy.TxIn[zeroIndex].Witness = witness
txCopy.TxIn[idx].PreviousOutPoint.Index = idx
Copy link
Member

Choose a reason for hiding this comment

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

So if we're not doing the 1-in-1-out version, then we don't need to swap out information for each input. Instead, the virtual tx has all the inputs, and idx here is just passed to sign/verify.

txCopy.TxIn[idx].Sequence = uint32(input.RelativeLockTime)
txCopy.TxIn[idx].Witness = witness
return txCopy
}

// VirtualGenesisTxIn computes the single input of a Taproot Asset virtual
// transaction that represents a grouped asset genesis. The input prevout's hash
// is the root of a MS-SMT committing to only the genesis asset.
func VirtualGenesisTxIn(newAsset *Asset) (*wire.TxIn, mssmt.Tree, error) {
func VirtualGenesisTxIn(newAsset *Asset) ([]*wire.TxIn, mssmt.Tree, error) {
inputTree := mssmt.NewCompactedTree(mssmt.NewDefaultStore())

// TODO(bhandras): thread the context through.
Expand Down Expand Up @@ -100,7 +100,11 @@ func VirtualGenesisTxIn(newAsset *Asset) (*wire.TxIn, mssmt.Tree, error) {

prevOut := VirtualTxInPrevOut(treeRoot)

return wire.NewTxIn(prevOut, nil, nil), inputTree, nil
txIns := []*wire.TxIn{
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this needs to change. There's only ever a single virtual txin still.

wire.NewTxIn(prevOut, nil, nil),
}

return txIns, inputTree, nil
}

// GenesisPrevOutFetcher returns a Taproot Asset input's `PrevOutFetcher` to be
Expand Down
2 changes: 1 addition & 1 deletion tapscript/mint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ func BuildGenesisTx(newAsset *asset.Asset) (*wire.MsgTx,

// Now, create the virtual transaction that represents this asset
// minting.
virtualTx, _, err := VirtualTx(newAsset, nil)
virtualTx, _, err := VirtualTx(newAsset, nil, []*asset.Asset{newAsset})
Copy link
Member

Choose a reason for hiding this comment

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

This looks a bit confusing, as if the inputs and outputs were the same... Maybe it would be a bit more clear if the first parameter would just be the []PreviousWitness slice, since it looks like only that is being used for the input part of the virtual TX?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah why do we need to pass in the asset twice? I don't see why we need to make any changes here. I think also shows the rationale re the design for the splits: the root split includes the commitment to them all, so you only need to pass around the root split for validation purposes.

if err != nil {
return nil, nil, err
}
Expand Down
9 changes: 7 additions & 2 deletions tapscript/send.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -648,9 +648,14 @@ func SignVirtualTransaction(vPkt *tappsbt.VPacket, signer Signer,
prevAssets[input.PrevID] = input.Asset()
}

assetOutputs := make([]*asset.Asset, len(outputs))
Copy link
Member

Choose a reason for hiding this comment

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

nit: could make use of the fn.Map() function here. Doesn't really make it shorter but makes review a bit easier IMO (since creation of the slice with make is abstracted away).

for idx := range outputs {
assetOutputs[idx] = outputs[idx].Asset
}

// Create a Taproot Asset virtual transaction representing the asset
// transfer.
virtualTx, _, err := VirtualTx(newAsset, prevAssets)
virtualTx, _, err := VirtualTx(newAsset, prevAssets, assetOutputs)
Copy link
Member

Choose a reason for hiding this comment

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

See above re asset outputs (they're already enumerated via the split commitment root).

if err != nil {
return err
}
Expand All @@ -669,7 +674,7 @@ func SignVirtualTransaction(vPkt *tappsbt.VPacket, signer Signer,
// Sign the virtual transaction based on the input script
// information (key spend or script spend).
newWitness, err := CreateTaprootSignature(
input, inputSpecificVirtualTx, 0, signer,
input, inputSpecificVirtualTx, idx, signer,
)
if err != nil {
return fmt.Errorf("error creating taproot "+
Expand Down
Loading
Loading