sweepbatcher: consolidate identical change pkscripts

batch separate change outputs with identical pkscripts
are tallied up and consolidated to a single change output.

sweepbatcher/greedy_batch_selection.go

@@ -210,11 +210,21 @@ func estimateBatchWeight(batch *batch) (feeDetails, error) {
 			err)
 	}
 
-	// Add change output weights.
+	// Add change output weights. Change outputs with identical pkscript
+	// will be consolidated into a single output.
+	changeOutputs := make(map[string]struct{})
 	for _, s := range batch.sweeps {
-		if s.change != nil {
-			weight.AddOutput(s.change.PkScript)
+		if s.change == nil {
+			continue
 		}
+
+		pkScriptString := string(s.change.PkScript)
+		if _, has := changeOutputs[pkScriptString]; has {
+			continue
+		}
+
+		weight.AddOutput(s.change.PkScript)
+		changeOutputs[pkScriptString] = struct{}{}
 	}
 
 	// Add inputs.

sweepbatcher/greedy_batch_selection_test.go

@@ -36,6 +36,7 @@ const (
 
 	coopTwoSweepBatchWeight          = coopNewBatchWeight + coopInputWeight
 	coopSingleSweepChangeBatchWeight = coopInputWeight + batchOutputWeight + changeOutputWeight
+	coopDoubleSweepChangeBatchWeight = 2*coopInputWeight + batchOutputWeight + changeOutputWeight
 	nonCoopTwoSweepBatchWeight       = coopTwoSweepBatchWeight + 2*nonCoopPenalty
 	v2v3BatchWeight                  = nonCoopTwoSweepBatchWeight - 25
 	mixedTwoSweepBatchWeight         = coopTwoSweepBatchWeight + nonCoopPenalty
@@ -325,6 +326,35 @@ func TestEstimateBatchWeight(t *testing.T) {
 			},
 		},
 
+		{
+			name: "two sweeps regular batch with identical change",
+			batch: &batch{
+				id: 1,
+				rbfCache: rbfCache{
+					FeeRate: lowFeeRate,
+				},
+				sweeps: map[wire.OutPoint]sweep{
+					outpoint1: {
+						htlcSuccessEstimator: se3,
+						change: &wire.TxOut{
+							PkScript: changePkscript,
+						},
+					},
+					outpoint2: {
+						htlcSuccessEstimator: se3,
+						change: &wire.TxOut{
+							PkScript: changePkscript,
+						},
+					},
+				},
+			},
+			wantBatchFeeDetails: feeDetails{
+				BatchId: 1,
+				FeeRate: lowFeeRate,
+				Weight:  coopDoubleSweepChangeBatchWeight,
+			},
+		},
+
 		{
 			name: "two sweeps regular batch",
 			batch: &batch{

sweepbatcher/sweep_batch.go

@@ -7,6 +7,7 @@ import (
 	"errors"
 	"fmt"
 	"math"
+	"sort"
 	"strings"
 	"sync"
 	"sync/atomic"
@@ -1311,10 +1312,22 @@ func constructUnsignedTx(sweeps []sweep, address btcutil.Address,
 		LockTime: uint32(currentHeight),
 	}
 
-	var changeOutputs []*wire.TxOut
-	for _, sweep := range sweeps {
-		if sweep.change != nil {
-			changeOutputs = append(changeOutputs, sweep.change)
+	// Consolidate change outputs with identical pkscript.
+	changeOutputs := make(map[string]*wire.TxOut)
+	for _, s := range sweeps {
+		if s.change == nil {
+			continue
+		}
+
+		stringPkScript := string(s.change.PkScript)
+		if _, has := changeOutputs[stringPkScript]; has {
+			changeOutputs[stringPkScript].Value += s.change.Value
+			continue
+		}
+
+		changeOutputs[stringPkScript] = &wire.TxOut{
+			Value:    s.change.Value,
+			PkScript: s.change.PkScript,
 		}
 	}
 
@@ -1425,11 +1438,25 @@ func constructUnsignedTx(sweeps []sweep, address btcutil.Address,
 		PkScript: batchPkScript,
 		Value:    int64(batchAmt-fee) - sumChange,
 	})
-	// Then add change outputs.
-	for _, txOut := range changeOutputs {
+	// Then add change outputs. Sort the keys first to make tests
+	// deterministic.
+	sortedChangeOutputs := make([]*wire.TxOut, 0, len(changeOutputs))
+	for _, output := range changeOutputs {
+		sortedChangeOutputs = append(sortedChangeOutputs, output)
+	}
+
+	// Sort the keys
+	sort.Slice(sortedChangeOutputs, func(i, j int) bool {
+		return utils.Bip69Less(
+			sortedChangeOutputs[i], sortedChangeOutputs[j],
+		)
+	})
+
+	// Add change outputs orderly.
+	for _, output := range sortedChangeOutputs {
 		batchTx.AddTxOut(&wire.TxOut{
-			PkScript: txOut.PkScript,
-			Value:    txOut.Value,
+			PkScript: output.PkScript,
+			Value:    output.Value,
 		})
 	}
 

sweepbatcher/sweep_batch_test.go

@@ -55,6 +55,17 @@ func TestConstructUnsignedTx(t *testing.T) {
 		PkScript: change1Pkscript,
 	}
 
+	change1PrimeAddr := "bc1pdx9ggvtjjcpaqfqk375qhdmzx9xu8dcu7w94lqfcxhh0rj" +
+		"lwyyeq5ryn6r"
+	change1PrimeAddress, err := btcutil.DecodeAddress(change1PrimeAddr, nil)
+	require.NoError(t, err)
+	change1PrimePkscript, err := txscript.PayToAddrScript(change1PrimeAddress)
+	require.NoError(t, err)
+	change1Prime := &wire.TxOut{
+		Value:    200_000,
+		PkScript: change1PrimePkscript,
+	}
+
 	change2Addr := "bc1psw0nrrulq4pgyuyk09a3wsutygltys4gxjjw3zl2uz4ep8pa" +
 		"r2vsvntfe0"
 	change2Address, err := btcutil.DecodeAddress(change2Addr, nil)
@@ -395,6 +406,108 @@ func TestConstructUnsignedTx(t *testing.T) {
 			wantFee:          1248,
 		},
 
+		{
+			name: "identical change pkscripts",
+			sweeps: []sweep{
+				{
+					outpoint: op1,
+					value:    1_000_000,
+				},
+				{
+					outpoint: op2,
+					value:    2_000_000,
+					change:   change1,
+				},
+				{
+					outpoint: op3,
+					value:    3_000_000,
+					change:   change1,
+				},
+			},
+			address:       p2trAddress,
+			currentHeight: 800_000,
+			feeRate:       1000,
+			wantTx: &wire.MsgTx{
+				Version:  2,
+				LockTime: 800_000,
+				TxIn: []*wire.TxIn{
+					{
+						PreviousOutPoint: op1,
+					},
+					{
+						PreviousOutPoint: op2,
+					},
+					{
+						PreviousOutPoint: op3,
+					},
+				},
+				TxOut: []*wire.TxOut{
+					{
+						Value:    5_798_924,
+						PkScript: p2trPkScript,
+					},
+					{
+						Value:    2 * change1.Value,
+						PkScript: change1.PkScript,
+					},
+				},
+			},
+			wantWeight:       1076,
+			wantFeeForWeight: 1076,
+			wantFee:          1076,
+		},
+
+		{
+			name: "identical change pkscripts different values",
+			sweeps: []sweep{
+				{
+					outpoint: op1,
+					value:    1_000_000,
+				},
+				{
+					outpoint: op2,
+					value:    2_000_000,
+					change:   change1,
+				},
+				{
+					outpoint: op3,
+					value:    3_000_000,
+					change:   change1Prime,
+				},
+			},
+			address:       p2trAddress,
+			currentHeight: 800_000,
+			feeRate:       1000,
+			wantTx: &wire.MsgTx{
+				Version:  2,
+				LockTime: 800_000,
+				TxIn: []*wire.TxIn{
+					{
+						PreviousOutPoint: op1,
+					},
+					{
+						PreviousOutPoint: op2,
+					},
+					{
+						PreviousOutPoint: op3,
+					},
+				},
+				TxOut: []*wire.TxOut{
+					{
+						Value:    5_698_924,
+						PkScript: p2trPkScript,
+					},
+					{
+						Value:    change1.Value + change1Prime.Value,
+						PkScript: change1.PkScript,
+					},
+				},
+			},
+			wantWeight:       1076,
+			wantFeeForWeight: 1076,
+			wantFee:          1076,
+		},
+
 		{
 			name: "change exceeds input value",
 			sweeps: []sweep{

sweepbatcher/sweep_batcher_presigned_test.go

@@ -1230,7 +1230,7 @@ func testPresigned_presigned_group_with_change(t *testing.T,
 	presignedHelper.SetOutpointOnline(op1, true)
 	presignedHelper.SetOutpointOnline(op2, true)
 
-	// An attempt to presign must fail.
+	// An attempt to presign shouldn't fail.
 	err = batcher.PresignSweepsGroup(
 		ctx, group1, sweepTimeout, destAddr, change,
 	)
@@ -1268,6 +1268,134 @@ func testPresigned_presigned_group_with_change(t *testing.T,
 	require.NoError(t, lnd.NotifyHeight(601))
 }
 
+// testPresigned_presigned_group_with_identical_change_pkscript tests passing multiple sweeps to
+// the method PresignSweepsGroup. It tests that a change output of a primary
+// deposit sweep is properly added to the presigned transaction.
+func testPresigned_presigned_group_with_identical_change_pkscript(t *testing.T,
+	batcherStore testBatcherStore) {
+
+	defer test.Guard(t)()
+
+	batchPkScript, err := txscript.PayToAddrScript(destAddr)
+	require.NoError(t, err)
+
+	lnd := test.NewMockLnd()
+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	customFeeRate := func(_ context.Context, _ lntypes.Hash,
+		_ wire.OutPoint) (chainfee.SatPerKWeight, error) {
+
+		return chainfee.SatPerKWeight(10_000), nil
+	}
+
+	presignedHelper := newMockPresignedHelper()
+
+	batcher := NewBatcher(
+		lnd.WalletKit, lnd.ChainNotifier, lnd.Signer,
+		testMuSig2SignSweep, testVerifySchnorrSig, lnd.ChainParams,
+		batcherStore, presignedHelper,
+		WithCustomFeeRate(customFeeRate),
+		WithPresignedHelper(presignedHelper),
+	)
+
+	go func() {
+		err := batcher.Run(ctx)
+		checkBatcherError(t, err)
+	}()
+
+	// Create two swaps of a single sweep
+	swapHash1 := lntypes.Hash{1, 1, 1}
+	swapHash2 := lntypes.Hash{2, 2, 2}
+	op1 := wire.OutPoint{
+		Hash:  chainhash.Hash{1, 1},
+		Index: 1,
+	}
+	op2 := wire.OutPoint{
+		Hash:  chainhash.Hash{2, 2},
+		Index: 2,
+	}
+	group1 := []Input{
+		{
+			Outpoint: op1,
+			Value:    1_000_000,
+		},
+	}
+	change1 := &wire.TxOut{
+		Value:    500_000,
+		PkScript: []byte{0xaf, 0xfe},
+	}
+	group2 := []Input{
+		{
+			Outpoint: op2,
+			Value:    2_000_000,
+		},
+	}
+	change2 := &wire.TxOut{
+		Value:    600_000,
+		PkScript: []byte{0xaf, 0xfe},
+	}
+
+	presignedHelper.setChangeForPrimaryDeposit(op1, change1)
+	presignedHelper.setChangeForPrimaryDeposit(op2, change2)
+
+	// Enable only one of the sweeps.
+	presignedHelper.SetOutpointOnline(op1, true)
+	presignedHelper.SetOutpointOnline(op2, true)
+
+	// An attempt to presign group1 shouldn't fail.
+	err = batcher.PresignSweepsGroup(
+		ctx, group1, sweepTimeout, destAddr, change1,
+	)
+	require.NoError(t, err)
+
+	// Add the sweep, triggering the publishing attempt.
+	err = batcher.AddSweep(ctx, &SweepRequest{
+		SwapHash: swapHash1,
+		Inputs:   group1,
+		Notifier: &dummyNotifier,
+	})
+	require.NoError(t, err)
+
+	// Since a batch was created we check that it registered for its primary
+	// sweep's spend.
+	<-lnd.RegisterSpendChannel
+
+	// An attempt to presign group2 shouldn't fail.
+	err = batcher.PresignSweepsGroup(
+		ctx, group2, sweepTimeout, destAddr, change2,
+	)
+	require.NoError(t, err)
+
+	// Add the sweep, triggering the publishing attempt.
+	err = batcher.AddSweep(ctx, &SweepRequest{
+		SwapHash: swapHash2,
+		Inputs:   group2,
+		Notifier: &dummyNotifier,
+	})
+	require.NoError(t, err)
+
+	// Wait for a transactions to be published.
+	tx := <-lnd.TxPublishChannel
+	require.Len(t, tx.TxIn, 2)
+	require.Len(t, tx.TxOut, 2)
+	require.ElementsMatch(
+		t, []wire.OutPoint{op1, op2},
+		[]wire.OutPoint{
+			tx.TxIn[0].PreviousOutPoint,
+			tx.TxIn[1].PreviousOutPoint,
+		},
+	)
+	require.Equal(t, int64(1_893_300), tx.TxOut[0].Value)
+	require.Equal(t, change1.Value+change2.Value, tx.TxOut[1].Value)
+	require.Equal(t, batchPkScript, tx.TxOut[0].PkScript)
+	require.Equal(t, change1.PkScript, tx.TxOut[1].PkScript)
+
+	// Mine a blocks to trigger republishing.
+	require.NoError(t, lnd.NotifyHeight(601))
+}
+
 // testPresigned_presigned_group_with_dust_main_output passes a dust main output
 // and a change output to PresignSweepsGroup. It will fail because of the dust
 // main output. Note that the min relay fee is set low enough to pass the
@@ -2224,6 +2352,10 @@ func TestPresigned(t *testing.T) {
 		testPresigned_presigned_group_with_change(t, NewStoreMock())
 	})
 
+	t.Run("identical change pkscript", func(t *testing.T) {
+		testPresigned_presigned_group_with_identical_change_pkscript(t, NewStoreMock())
+	})
+
 	t.Run("dust_main_output", func(t *testing.T) {
 		testPresigned_presigned_group_with_dust_main_output(
 			t, NewStoreMock(),