From 07005231db60f631201d55ea71f61074f0d8abe3 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Wed, 14 May 2025 16:01:44 +0200 Subject: [PATCH] tapchannel: don't break backward compatibility This partially reverts commit 3ed142ffdc59f53a3d405b960f5d24641c5cc51c 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). --- tapchannel/aux_closer.go | 7 ++- tapchannel/commitment.go | 23 ++++++- tapsend/allocation_test.go | 123 +++++++++++++++++++++++++++++++++++++ 3 files changed, 149 insertions(+), 4 deletions(-) diff --git a/tapchannel/aux_closer.go b/tapchannel/aux_closer.go index 6d503fdcb..d5ec0d482 100644 --- a/tapchannel/aux_closer.go +++ b/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 diff --git a/tapchannel/commitment.go b/tapchannel/commitment.go index a562b26e6..fa4b257fc 100644 --- a/tapchannel/commitment.go +++ b/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, diff --git a/tapsend/allocation_test.go b/tapsend/allocation_test.go index c604a475b..5d816b039 100644 --- a/tapsend/allocation_test.go +++ b/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 {