Merge pull request #1537 from lightninglabs/backward-compat-revert

tapchannel: don't break backward compatibility

Author: Roasbeef

tapchannel/aux_closer.go

@@ -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,
@@ -146,6 +146,7 @@ func createCloseAlloc(isLocal bool, outputSum uint64,
 
 			return tapsend.CommitAllocationToRemote
 		}(),
+		SplitRoot:            isInitiator,
 		InternalKey:          shutdownMsg.AssetInternalKey.Val,
 		GenScriptKey:         scriptKeyGen,
 		Amount:               outputSum,
@@ -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
@@ -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

tapchannel/commitment.go

@@ -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,
@@ -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
@@ -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,
@@ -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,
@@ -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,

tapsend/allocation_test.go

@@ -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,
+						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 {