tapchannel: don't break backward compatibility

This partially reverts commit 3ed142f
to restore backward compatibility with nodes running on v0.5.x.

The intention with removing this explicit split root assignment was to
simplify the code. But it actually would lead to force closes between
nodes on v0.6.0 and v0.5.x, because they wouldn't be able to agree on
what output should be chosen for the split root (since the behavior was
changed).

Author: guggero

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 {