Skip to content

[wallet 3/3]: group key support for channel funding #1413

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.

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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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: 1 addition & 1 deletion .github/workflows/main.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ env:

GO_VERSION: '1.23.6'

LITD_ITEST_BRANCH: 'closedchannel-data'
LITD_ITEST_BRANCH: 'group-key-support'

jobs:
#######################
Expand Down
13 changes: 12 additions & 1 deletion rpcserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -3561,6 +3561,16 @@ func marshalOutboundParcel(
return nil, err
}

var proofAsset asset.Asset
err = proof.SparseDecode(
bytes.NewReader(out.ProofSuffix),
proof.AssetLeafRecord(&proofAsset),
)
if err != nil {
return nil, fmt.Errorf("unable to sparse decode "+
"proof: %w", err)
}

// Marshall the proof delivery status.
proofDeliveryStatus := marshalOutputProofDeliveryStatus(out)

Expand All @@ -3576,6 +3586,7 @@ func marshalOutboundParcel(
OutputType: rpcOutType,
AssetVersion: assetVersion,
ProofDeliveryStatus: proofDeliveryStatus,
AssetId: fn.ByteSlice(proofAsset.ID()),
}
}

Expand Down Expand Up @@ -7099,7 +7110,7 @@ func (r *rpcServer) FundChannel(ctx context.Context,
}

assetID, groupKey, err := parseAssetSpecifier(
req.GetAssetId(), "", nil, "",
req.GetAssetId(), "", req.GetGroupKey(), "",
Copy link
Member

Choose a reason for hiding this comment

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

Re the other comment, this is where we can implement the either or behavior:

  • We only allow asset ID or group key to be set
  • We make a specifier that only has one half populated

With that change, I don't think we'd need that new bool and behavior change in the db?

Copy link
Member

Choose a reason for hiding this comment

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

added something similar in 667080c

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, yeah, maybe we can introduce a sub type ExclusiveSpecifier that embeds the original identifier (just so we don't need to change things everywhere)... Will think about it.

)
if err != nil {
return nil, fmt.Errorf("error parsing asset specifier: %w", err)
Expand Down
153 changes: 80 additions & 73 deletions tapchannel/aux_closer.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ func NewAuxChanCloser(cfg AuxChanCloserCfg) *AuxChanCloser {
// createCloseAlloc is a helper function that creates an allocation for an asset
// close. This does not set a script key, as the script key will be set for each
// packet after the coins have been distributed.
func createCloseAlloc(isLocal, isInitiator bool, outputSum uint64,
func createCloseAlloc(isLocal bool, outputSum uint64,
shutdownMsg tapchannelmsg.AuxShutdownMsg) (*tapsend.Allocation, error) {

// The sort pkScript for the allocation will just be the internal key,
Expand Down Expand Up @@ -146,7 +146,6 @@ func createCloseAlloc(isLocal, isInitiator bool, outputSum uint64,

return tapsend.CommitAllocationToRemote
}(),
SplitRoot: isInitiator,
InternalKey: shutdownMsg.AssetInternalKey.Val,
GenScriptKey: scriptKeyGen,
Amount: outputSum,
Expand All @@ -157,24 +156,58 @@ func createCloseAlloc(isLocal, isInitiator bool, outputSum uint64,
}, nil
}

// fundingSpendWitness creates a complete witness to spend the OP_TRUE funding
// script of an asset funding output.
func fundingSpendWitness() lfn.Result[wire.TxWitness] {
fundingScriptTree := tapscript.NewChannelFundingScriptTree()
// signCommitVirtualPackets signs the commit virtual packets with the funding
// witness, which is just the script and control block for the OP_TRUE spend.
func signCommitVirtualPackets(ctx context.Context,
vPackets []*tappsbt.VPacket) error {

tapscriptTree := fundingScriptTree.TapscriptTree
ctrlBlock := tapscriptTree.LeafMerkleProofs[0].ToControlBlock(
&input.TaprootNUMSKey,
)
ctrlBlockBytes, err := ctrlBlock.ToBytes()
if err != nil {
return lfn.Errf[wire.TxWitness]("unable to serialize control "+
"block: %w", err)
useUniqueScriptKey := len(vPackets) > 1
Copy link
Member

Choose a reason for hiding this comment

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

we assume that two different vPacket's may not use the same asset ID?
if yes could throw a comment for clarity

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. The definition for a vPacket that we arrived at is that it reflects all the changes of a single assetID within one on-chain transaction. Meaning that all the inputs and all the outputs of one asset ID within a transfer would be merged into the same vPacket. Will add a comment to make more clear.

for idx := range vPackets {
assetID, err := vPackets[idx].AssetID()
if err != nil {
return fmt.Errorf("unable to get asset ID: %w", err)
}

// First, we'll prepare the funding witness which includes the
// OP_TRUE ctrl block.
fundingWitness, err := tapscript.ChannelFundingSpendWitness(
useUniqueScriptKey, assetID,
)
if err != nil {
return fmt.Errorf("unable to make funding witness: %w",
err)
}

err = tapsend.PrepareOutputAssets(ctx, vPackets[idx])
if err != nil {
return fmt.Errorf("unable to prepare output "+
"assets: %w", err)
}

// With the packets prepared, we'll swap in the correct witness
// for each of them. We need to do this _after_ calling
// PrepareOutputAsset, because that method will overwrite any
// asset in the virtual outputs. Which means we'll also need to
// set the witness on _every_ output of the packet, to make sure
// each split output's root asset reference also gets the
// correct witness.
for outIdx := range vPackets[idx].Outputs {
outAsset := vPackets[idx].Outputs[outIdx].Asset

// There is always only a single input, as we're
// spending a single funding output w/ each vPkt.
const inputIndex = 0
err := outAsset.UpdateTxWitness(
inputIndex, fundingWitness,
)
if err != nil {
return fmt.Errorf("error updating witness: %w",
err)
}
}
}

return lfn.Ok(wire.TxWitness{
tapscript.AnyoneCanSpendScript(), ctrlBlockBytes,
})
return nil
}

// AuxCloseOutputs returns the set of close outputs to use for this co-op close
Expand Down Expand Up @@ -273,7 +306,7 @@ func (a *AuxChanCloser) AuxCloseOutputs(
remoteSum := fn.Reduce(commitState.RemoteAssets.Val.Outputs, sumAmounts)
if localSum > 0 {
localAlloc, err = createCloseAlloc(
true, desc.Initiator, localSum, localShutdown,
true, localSum, localShutdown,
)
if err != nil {
return none, err
Expand All @@ -285,7 +318,7 @@ func (a *AuxChanCloser) AuxCloseOutputs(
}
if remoteSum > 0 {
remoteAlloc, err = createCloseAlloc(
false, !desc.Initiator, remoteSum, remoteShutdown,
false, remoteSum, remoteShutdown,
)
if err != nil {
return none, err
Expand Down Expand Up @@ -378,35 +411,12 @@ func (a *AuxChanCloser) AuxCloseOutputs(
return none, fmt.Errorf("unable to distribute coins: %w", err)
}

// With the vPackets created we'll now prepare all the split
// information encoded in the vPackets.
fundingWitness, err := fundingSpendWitness().Unpack()
if err != nil {
return none, fmt.Errorf("unable to make funding "+
"witness: %w", err)
}
ctx := context.Background()
for idx := range vPackets {
err := tapsend.PrepareOutputAssets(ctx, vPackets[idx])
if err != nil {
return none, fmt.Errorf("unable to prepare output "+
"assets: %w", err)
}

for outIdx := range vPackets[idx].Outputs {
outAsset := vPackets[idx].Outputs[outIdx].Asset

// There is always only a single input, which is the
// funding output.
const inputIndex = 0
err := outAsset.UpdateTxWitness(
inputIndex, fundingWitness,
)
if err != nil {
return none, fmt.Errorf("error updating "+
"witness: %w", err)
}
}
// We can now add the witness for the OP_TRUE spend of the commitment
// output to the vPackets.
ctxb := context.Background()
if err := signCommitVirtualPackets(ctxb, vPackets); err != nil {
return none, fmt.Errorf("error signing commit virtual "+
"packets: %w", err)
}

// With the outputs prepared, we can now create the set of output
Expand Down Expand Up @@ -484,8 +494,9 @@ func (a *AuxChanCloser) ShutdownBlob(
none := lfn.None[lnwire.CustomRecords]()

// If there's no custom blob, then we don't need to do anything.
if req.CommitBlob.IsNone() {
log.Debugf("No commit blob for ChannelPoint(%v)", req.ChanPoint)
if req.FundingBlob.IsNone() {
log.Debugf("No funding blob for ChannelPoint(%v)",
req.ChanPoint)
return none, nil
}

Expand All @@ -500,16 +511,16 @@ func (a *AuxChanCloser) ShutdownBlob(
log.Infof("Creating shutdown blob for close of ChannelPoint(%v)",
req.ChanPoint)

// Otherwise, we'll decode the commitment, so we can examine the current
// state.
var commitState tapchannelmsg.Commitment
err = lfn.MapOptionZ(req.CommitBlob, func(blob tlv.Blob) error {
c, err := tapchannelmsg.DecodeCommitment(blob)
// Otherwise, we'll decode the funding state, so we can examine the
// different asset IDs in the channel.
var fundingState tapchannelmsg.OpenChannel
err = lfn.MapOptionZ(req.FundingBlob, func(blob tlv.Blob) error {
c, err := tapchannelmsg.DecodeOpenChannel(blob)
if err != nil {
return err
}

commitState = *c
fundingState = *c

return nil
})
Expand All @@ -528,38 +539,34 @@ func (a *AuxChanCloser) ShutdownBlob(
return none, err
}

// Next, we'll collect all the assets that we own in this channel.
assets := commitState.LocalAssets.Val.Outputs
// Next, we'll collect all the asset IDs that were committed to the
// channel.
assetIDs := fn.Map(
Copy link
Member

Choose a reason for hiding this comment

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

IIUC, there can potentially be duplicates here: multiple UTXOs of a given asset ID were used to fund the channel? Perhaps it should be run through as a set first.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I don't think that should ever be the case. Even if you have multiple inputs of the same asset ID, we'd merge them into the same UTXO. Only for different asset IDs we need to keep distinct UTXOs around (because they'll be represented by distinct vPackets). Will add a comment to make that more clear.

fundingState.FundedAssets.Val.Outputs,
func(o *tapchannelmsg.AssetOutput) asset.ID {
return o.AssetID.Val
},
)

// Now that we have all the asset IDs, we'll query for a new key for
// each of them which we'll use as both the internal key and the script
// key.
scriptKeys := make(tapchannelmsg.ScriptKeyMap)
for idx := range assets {
channelAsset := assets[idx]

for _, assetID := range assetIDs {
newKey, err := a.cfg.AddrBook.NextScriptKey(
ctx, asset.TaprootAssetsKeyFamily,
)
if err != nil {
return none, err
}

// We now add the a
// TODO(guggero): This only works if there's only a single asset
// in the channel. We need to extend this to support multiple
// assets.
_, err = a.cfg.AddrBook.NewAddressWithKeys(
ctx, address.V1, channelAsset.AssetID.Val,
channelAsset.Amount.Val, newKey, newInternalKey, nil,
*a.cfg.DefaultCourierAddr,
)
err = a.cfg.AddrBook.InsertScriptKey(ctx, newKey, true)
if err != nil {
return none, fmt.Errorf("error adding new address: %w",
err)
return none, fmt.Errorf("error declaring script key: "+
"%w", err)
}

scriptKeys[channelAsset.AssetID.Val] = *newKey.PubKey
scriptKeys[assetID] = *newKey.PubKey
Copy link
Member

Choose a reason for hiding this comment

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

Based on the comment above, we may end up with script keys thgat was created/inserted but were never used as we overrode them here in the map.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, you're right... So perhaps for the very first one we should try to re-use the one we already derived?

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, my comment right above this was incorrect. Because we shouldn't have duplicate asset IDs (as explained in #1413 (comment)), we shouldn't overwrite keys here. So we don't derive keys that we then don't end up using.

}

// Finally, we'll map the extra shutdown info to a TLV record map we
Expand Down
Loading
Loading