diff --git a/sweepbatcher/greedy_batch_selection.go b/sweepbatcher/greedy_batch_selection.go index 8b910dbc..1ecda9b0 100644 --- a/sweepbatcher/greedy_batch_selection.go +++ b/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. diff --git a/sweepbatcher/greedy_batch_selection_test.go b/sweepbatcher/greedy_batch_selection_test.go index 51dd70d2..97c087f1 100644 --- a/sweepbatcher/greedy_batch_selection_test.go +++ b/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{ diff --git a/sweepbatcher/sweep_batch.go b/sweepbatcher/sweep_batch.go index bebe2118..47deb2be 100644 --- a/sweepbatcher/sweep_batch.go +++ b/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, }) } diff --git a/sweepbatcher/sweep_batch_test.go b/sweepbatcher/sweep_batch_test.go index 202e0a04..34cdb87e 100644 --- a/sweepbatcher/sweep_batch_test.go +++ b/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{ diff --git a/sweepbatcher/sweep_batcher_presigned_test.go b/sweepbatcher/sweep_batcher_presigned_test.go index 99a70fd4..a3849627 100644 --- a/sweepbatcher/sweep_batcher_presigned_test.go +++ b/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(),