Skip to content

tapchannel: don't break backward compatibility #1537

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

Merged
merged 1 commit into from
May 15, 2025
Merged

tapchannel: don't break backward compatibility #1537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 4 additions & 3 deletions tapchannel/aux_closer.go
View file
Open in desktop
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 bool, outputSum uint64,
func createCloseAlloc(isLocal, isInitiator 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,6 +146,7 @@ func createCloseAlloc(isLocal bool, outputSum uint64,

return tapsend.CommitAllocationToRemote
}(),
SplitRoot: isInitiator,
InternalKey: shutdownMsg.AssetInternalKey.Val,
GenScriptKey: scriptKeyGen,
Amount: outputSum,
Expand Down Expand Up @@ -306,7 +307,7 @@ func (a *AuxChanCloser) AuxCloseOutputs(
remoteSum := fn.Reduce(commitState.RemoteAssets.Val.Outputs, sumAmounts)
if localSum > 0 {
localAlloc, err = createCloseAlloc(
true, localSum, localShutdown,
true, desc.Initiator, localSum, localShutdown,
)
if err != nil {
return none, err
Expand All @@ -318,7 +319,7 @@ func (a *AuxChanCloser) AuxCloseOutputs(
}
if remoteSum > 0 {
remoteAlloc, err = createCloseAlloc(
false, remoteSum, remoteShutdown,
false, !desc.Initiator, remoteSum, remoteShutdown,
)
if err != nil {
return none, err
Expand Down
23 changes: 22 additions & 1 deletion tapchannel/commitment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -714,7 +714,10 @@ func CreateAllocations(chanState lnwallet.AuxChanState, ourBalance,
initiatorAssetBalance)

// Next, we add the HTLC outputs, using this helper function to
// distinguish between incoming and outgoing HTLCs.
// distinguish between incoming and outgoing HTLCs. The haveHtlcSplit
// boolean is used to store if one of the HTLCs has already been chosen
// to be the split root (only the very first HTLC might be chosen).
var haveHtlcSplitRoot bool
addHtlc := func(htlc *DecodedDescriptor, isIncoming bool) error {
htlcScript, err := lnwallet.GenTaprootHtlcScript(
isIncoming, whoseCommit, htlc.Timeout, htlc.RHash,
Expand All @@ -732,6 +735,21 @@ func CreateAllocations(chanState lnwallet.AuxChanState, ourBalance,
"sibling: %w", err)
}

// We should always just have a single split root, which
// normally is the initiator's balance. However, if the
// initiator has no balance, then we choose the very first HTLC
// in the list to be the split root. If there are no HTLCs, then
// all the balance is on the receiver side and we don't need a
// split root.
shouldHouseSplitRoot := initiatorAssetBalance == 0 &&
!haveHtlcSplitRoot

// Make sure we only select the very first HTLC that pays to the
// initiator.
if shouldHouseSplitRoot {
haveHtlcSplitRoot = true
}

allocType := tapsend.CommitAllocationHtlcOutgoing
if isIncoming {
allocType = tapsend.CommitAllocationHtlcIncoming
Expand Down Expand Up @@ -779,6 +797,7 @@ func CreateAllocations(chanState lnwallet.AuxChanState, ourBalance,
Type: allocType,
Amount: rfqmsg.Sum(htlc.AssetBalances),
AssetVersion: asset.V1,
SplitRoot: shouldHouseSplitRoot,
BtcAmount: htlc.Amount.ToSatoshis(),
InternalKey: htlcTree.InternalKey,
NonAssetLeaves: sibling,
Expand Down Expand Up @@ -982,6 +1001,7 @@ func addCommitmentOutputs(chanType channeldb.ChannelType, localChanCfg,
Type: tapsend.CommitAllocationToLocal,
Amount: ourAssetBalance,
AssetVersion: asset.V1,
SplitRoot: initiator,
BtcAmount: ourBalance,
InternalKey: toLocalTree.InternalKey,
NonAssetLeaves: sibling,
Expand Down Expand Up @@ -1044,6 +1064,7 @@ func addCommitmentOutputs(chanType channeldb.ChannelType, localChanCfg,
Type: tapsend.CommitAllocationToRemote,
Amount: theirAssetBalance,
AssetVersion: asset.V1,
SplitRoot: !initiator,
BtcAmount: theirBalance,
InternalKey: toRemoteTree.InternalKey,
NonAssetLeaves: sibling,
Expand Down
123 changes: 123 additions & 0 deletions tapsend/allocation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -877,6 +877,129 @@ func TestDistributeCoins(t *testing.T) {
},
},
},
{
// This tests the backward compatibility case where the
// split root output is defined explicitly, even for an
// interactive packet (which is the case for channels).
name: "single asset, distributed to three outputs",
inputs: []*proof.Proof{
makeProof(t, assetID1Tranche1),
},
interactive: true,
//nolint:lll
allocations: []*Allocation{
{
Type: CommitAllocationHtlcOutgoing,
Amount: 50,
OutputIndex: 2,
},
{
Type: CommitAllocationToLocal,
Amount: 20,
OutputIndex: 3,
},
{
Type: CommitAllocationToRemote,
Amount: 30,
OutputIndex: 4,
SplitRoot: true,
},
},
expectedInputs: map[asset.ID][]asset.ScriptKey{
assetID1.ID(): {
assetID1Tranche1.ScriptKey,
},
},
expectedOutputs: map[asset.ID][]*tappsbt.VOutput{
assetID1.ID(): {
{
Amount: 50,
Type: simple,
Interactive: true,
AnchorOutputIndex: 2,
},
{
Amount: 20,
Type: simple,
Interactive: true,
AnchorOutputIndex: 3,
},
{
Amount: 30,
Type: split,
Interactive: true,
AnchorOutputIndex: 4,
},
},
},
},
{
// This shows that for channels with multiple asset IDs,
// defining the split root output might not be enough,
// if some assets aren't even distributed to that
// output. So the fallback for a packet that doesn't
// get an exact split root output is to just use the
// first output as the split root.
name: "multiple allocations, split root defined on " +
"output that gets full value",
inputs: []*proof.Proof{
makeProof(t, assetID4Tranche1),
makeProof(t, assetID5Tranche1),
},
interactive: true,
//nolint:lll
allocations: []*Allocation{
{
Type: CommitAllocationHtlcOutgoing,
Amount: 5000,
OutputIndex: 2,
},
{
Type: CommitAllocationToLocal,
Amount: 20000,
OutputIndex: 3,
},
{
Type: CommitAllocationToRemote,
Amount: 25000,
OutputIndex: 4,
SplitRoot: true,
},
},
vPktVersion: tappsbt.V1,
expectedInputs: map[asset.ID][]asset.ScriptKey{
assetID4.ID(): {
assetID4Tranche1.ScriptKey,
},
assetID5.ID(): {
assetID5Tranche1.ScriptKey,
},
},
expectedOutputs: map[asset.ID][]*tappsbt.VOutput{
assetID4.ID(): {
{
Amount: 5000,
Type: split,
Copy link
Member

@Roasbeef Roasbeef May 15, 2025

Choose a reason for hiding this comment

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

So this output is the split because it has a lower output index, even though we have a settled to-self output present in the commitment?

Copy link
Contributor Author

@guggero guggero May 15, 2025

Choose a reason for hiding this comment

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

This shows why the reverted commit was added initially: If there are multiple asset pieces in a channel, you can't guarantee that every on-chain output gets assets from every piece (depending on the amount distribution). So even if you say the to_remote output should house split root, there might be virtual packets (in this example asset ID 4) will not have any split output in the to_remote output.
In that case we just fall back to having the split at index 0.

But, since this new fallback case can only happen for new, grouped asset channels, it's okay since both nodes need to be on the newer version to understand.
And for the cases where there's only one asset in the channel, we know that each output will get a piece of that asset and we can guarantee that the split output is at the location it was in previous versions, hence restoring backward compatibility.

Interactive: true,
AnchorOutputIndex: 2,
},
{
Amount: 20000,
Type: simple,
Interactive: true,
AnchorOutputIndex: 3,
},
},
assetID5.ID(): {
{
Amount: 25000,
Type: simple,
Interactive: true,
AnchorOutputIndex: 4,
},
},
},
},
}

for _, tc := range testCases {
Expand Down
Loading