Merge pull request #1020 from starius/sweepbatcher-change-outputs-fees-fix

sweepbatcher: fix change fee accounting, add test

sweepbatcher/presigned.go

@@ -506,7 +506,10 @@ func (b *batch) publishPresigned(ctx context.Context) (btcutil.Amount, error,
 
 	// Find actual fee rate of the signed transaction. It may differ from
 	// the desired fee rate, because SignTx may return a presigned tx.
-	output := btcutil.Amount(tx.TxOut[0].Value)
+	var output btcutil.Amount
+	for _, txOut := range tx.TxOut {
+		output += btcutil.Amount(txOut.Value)
+	}
 	fee = batchAmt - output
 	signedFeeRate := chainfee.NewSatPerKWeight(fee, realWeight)
 

sweepbatcher/sweep_batch.go

@@ -2071,8 +2071,8 @@ func getFeePortionForSweep(spendTx *wire.MsgTx, numSweeps int,
 	totalSweptAmt btcutil.Amount) (btcutil.Amount, btcutil.Amount) {
 
 	totalFee := int64(totalSweptAmt)
-	if len(spendTx.TxOut) > 0 {
-		totalFee -= spendTx.TxOut[0].Value
+	for _, txOut := range spendTx.TxOut {
+		totalFee -= txOut.Value
 	}
 	feePortionPerSweep := totalFee / int64(numSweeps)
 	roundingDiff := totalFee - (int64(numSweeps) * feePortionPerSweep)

sweepbatcher/sweep_batcher_presigned_test.go

@@ -1,6 +1,7 @@
 package sweepbatcher
 
 import (
+	"bytes"
 	"context"
 	"fmt"
 	"os"
@@ -1268,6 +1269,156 @@ func testPresigned_presigned_group_with_change(t *testing.T,
 	require.NoError(t, lnd.NotifyHeight(601))
 }
 
+// testPresigned_fee_portion_with_change ensures that the fee portion reported
+// to clients accounts for change outputs in the presigned transaction. It also
+// is a regression test for feerate overestimation when tx is published.
+func testPresigned_fee_portion_with_change(t *testing.T,
+	batcherStore testBatcherStore) {
+
+	defer test.Guard(t)()
+
+	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)
+	}()
+
+	swapHash := lntypes.Hash{2, 2, 2}
+	op := wire.OutPoint{
+		Hash:  chainhash.Hash{2, 2},
+		Index: 2,
+	}
+	group := []Input{
+		{
+			Outpoint: op,
+			Value:    1_000_000,
+		},
+	}
+	change := &wire.TxOut{
+		Value:    250_000,
+		PkScript: []byte{0xca, 0xfe},
+	}
+
+	presignedHelper.setChangeForPrimaryDeposit(op, change)
+	presignedHelper.SetOutpointOnline(op, true)
+
+	require.NoError(t, batcher.PresignSweepsGroup(
+		ctx, group, sweepTimeout, destAddr, change,
+	))
+
+	spendChan := make(chan *SpendDetail, 1)
+	confChan := make(chan *ConfDetail, 1)
+	notifier := &SpendNotifier{
+		SpendChan:    spendChan,
+		SpendErrChan: make(chan error, 1),
+		ConfChan:     confChan,
+		ConfErrChan:  make(chan error, 1),
+		QuitChan:     make(chan bool, 1),
+	}
+
+	require.NoError(t, batcher.AddSweep(ctx, &SweepRequest{
+		SwapHash: swapHash,
+		Inputs:   group,
+		Notifier: notifier,
+	}))
+
+	spendReg := <-lnd.RegisterSpendChannel
+	require.NotNil(t, spendReg)
+	require.NotNil(t, spendReg.Outpoint)
+	require.Equal(t, op, *spendReg.Outpoint)
+
+	tx := <-lnd.TxPublishChannel
+	require.Len(t, tx.TxIn, 1)
+	require.Len(t, tx.TxOut, 2)
+
+	// Mine a blocks to trigger republishing.
+	require.NoError(t, lnd.NotifyHeight(601))
+
+	// Make sure it is the same tx.
+	tx2 := <-lnd.TxPublishChannel
+	require.Len(t, tx2.TxOut, len(tx.TxOut))
+	require.Equal(t, tx.TxOut[0].Value, tx2.TxOut[0].Value)
+
+	var (
+		outputSum   int64
+		foundChange bool
+	)
+	for _, txOut := range tx.TxOut {
+		outputSum += txOut.Value
+		if txOut.Value != change.Value {
+			continue
+		}
+
+		if !bytes.Equal(txOut.PkScript, change.PkScript) {
+			continue
+		}
+
+		foundChange = true
+	}
+
+	require.True(t, foundChange)
+
+	totalInput := int64(group[0].Value)
+	require.LessOrEqual(t, outputSum, totalInput)
+
+	expectedFee := btcutil.Amount(totalInput - outputSum)
+	require.Greater(t, expectedFee, btcutil.Amount(0))
+
+	txHash := tx.TxHash()
+	spendDetail := &chainntnfs.SpendDetail{
+		SpentOutPoint:     &op,
+		SpendingTx:        tx,
+		SpenderTxHash:     &txHash,
+		SpenderInputIndex: 0,
+		SpendingHeight:    spendReg.HeightHint + 1,
+	}
+	lnd.SpendChannel <- spendDetail
+
+	spend := <-spendChan
+	require.Equal(t, expectedFee, spend.OnChainFeePortion)
+
+	confReg := <-lnd.RegisterConfChannel
+	require.True(t, bytes.Equal(tx.TxOut[0].PkScript, confReg.PkScript) ||
+		bytes.Equal(tx.TxOut[1].PkScript, confReg.PkScript))
+
+	require.NoError(
+		t, lnd.NotifyHeight(spendReg.HeightHint+batchConfHeight+1),
+	)
+	lnd.ConfChannel <- &chainntnfs.TxConfirmation{Tx: tx}
+
+	require.Eventually(t, func() bool {
+		select {
+		case <-presignedHelper.cleanupCalled:
+			return true
+		default:
+			return false
+		}
+	}, test.Timeout, eventuallyCheckFrequency)
+
+	conf := <-confChan
+	require.Equal(t, expectedFee, conf.OnChainFeePortion)
+}
+
 // 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.
@@ -2356,6 +2507,10 @@ func TestPresigned(t *testing.T) {
 		testPresigned_presigned_group_with_change(t, NewStoreMock())
 	})
 
+	t.Run("fee_portion_change", func(t *testing.T) {
+		testPresigned_fee_portion_with_change(t, NewStoreMock())
+	})
+
 	t.Run("identical change pkscript", func(t *testing.T) {
 		testPresigned_presigned_group_with_identical_change_pkscript(t, NewStoreMock())
 	})