Merge pull request #4588 from bjarnemagnussen/coinselect-align-fee-estimates

chanfunding: fee estimation based on change output in `CoinSelect`

lnwallet/chanfunding/coin_select.go

@@ -25,6 +25,18 @@ func (e *ErrInsufficientFunds) Error() string {
 		e.amountAvailable, e.amountSelected)
 }
 
+// errUnsupportedInput is a type matching the error interface, which is returned
+// when trying to calculate the fee of a transaction that references an
+// unsupported script in the outpoint of a transaction input.
+type errUnsupportedInput struct {
+	PkScript []byte
+}
+
+// Error returns a human readable string describing the error.
+func (e *errUnsupportedInput) Error() string {
+	return fmt.Sprintf("unsupported address type: %x", e.PkScript)
+}
+
 // Coin represents a spendable UTXO which is available for channel funding.
 // This UTXO need not reside in our internal wallet as an example, and instead
 // may be derived from an existing watch-only wallet. It wraps both the output
@@ -52,11 +64,65 @@ func selectInputs(amt btcutil.Amount, coins []Coin) (btcutil.Amount, []Coin, err
 	return 0, nil, &ErrInsufficientFunds{amt, satSelected}
 }
 
+// calculateFees returns for the specified utxos and fee rate two fee
+// estimates, one calculated using a change output and one without. The weight
+// added to the estimator from a change output is for a P2WKH output.
+func calculateFees(utxos []Coin, feeRate chainfee.SatPerKWeight) (btcutil.Amount,
+	btcutil.Amount, error) {
+
+	var weightEstimate input.TxWeightEstimator
+	for _, utxo := range utxos {
+		switch {
+
+		case txscript.IsPayToWitnessPubKeyHash(utxo.PkScript):
+			weightEstimate.AddP2WKHInput()
+
+		case txscript.IsPayToScriptHash(utxo.PkScript):
+			weightEstimate.AddNestedP2WKHInput()
+
+		default:
+			return 0, 0, &errUnsupportedInput{utxo.PkScript}
+		}
+	}
+
+	// Channel funding multisig output is P2WSH.
+	weightEstimate.AddP2WSHOutput()
+
+	// Estimate the fee required for a transaction without a change
+	// output.
+	totalWeight := int64(weightEstimate.Weight())
+	requiredFeeNoChange := feeRate.FeeForWeight(totalWeight)
+
+	// Estimate the fee required for a transaction with a change output.
+	// Assume that change output is a P2WKH output.
+	weightEstimate.AddP2WKHOutput()
+
+	// Now that we have added the change output, redo the fee
+	// estimate.
+	totalWeight = int64(weightEstimate.Weight())
+	requiredFeeWithChange := feeRate.FeeForWeight(totalWeight)
+
+	return requiredFeeNoChange, requiredFeeWithChange, nil
+}
+
+// sanityCheckFee checks if the specified fee amounts to over 20% of the total
+// output amount and raises an error.
+func sanityCheckFee(totalOut, fee btcutil.Amount) error {
+	// Fail if more than 20% goes to fees.
+	// TODO(halseth): smarter fee limit. Make configurable or dynamic wrt
+	// total funding size?
+	if fee > totalOut/5 {
+		return fmt.Errorf("fee %v on total output value %v", fee,
+			totalOut)
+	}
+	return nil
+}
+
 // CoinSelect attempts to select a sufficient amount of coins, including a
 // change output to fund amt satoshis, adhering to the specified fee rate. The
 // specified fee rate should be expressed in sat/kw for coin selection to
 // function properly.
-func CoinSelect(feeRate chainfee.SatPerKWeight, amt btcutil.Amount,
+func CoinSelect(feeRate chainfee.SatPerKWeight, amt, dustLimit btcutil.Amount,
 	coins []Coin) ([]Coin, btcutil.Amount, error) {
 
 	amtNeeded := amt
@@ -68,53 +134,57 @@ func CoinSelect(feeRate chainfee.SatPerKWeight, amt btcutil.Amount,
 			return nil, 0, err
 		}
 
-		var weightEstimate input.TxWeightEstimator
+		// Obtain fee estimates both with and without using a change
-
+		// output.
-		for _, utxo := range selectedUtxos {
+		requiredFeeNoChange, requiredFeeWithChange, err := calculateFees(
-			switch {
+			selectedUtxos, feeRate,
-
+		)
-			case txscript.IsPayToWitnessPubKeyHash(utxo.PkScript):
+		if err != nil {
-				weightEstimate.AddP2WKHInput()
+			return nil, 0, err
-
-			case txscript.IsPayToScriptHash(utxo.PkScript):
-				weightEstimate.AddNestedP2WKHInput()
-
-			default:
-				return nil, 0, fmt.Errorf("unsupported address type: %x",
-					utxo.PkScript)
 		}
-		}
-
-		// Channel funding multisig output is P2WSH.
-		weightEstimate.AddP2WSHOutput()
-
-		// Assume that change output is a P2WKH output.
-		//
-		// TODO: Handle wallets that generate non-witness change
-		// addresses.
-		// TODO(halseth): make coinSelect not estimate change output
-		// for dust change.
-		weightEstimate.AddP2WKHOutput()
 
 		// The difference between the selected amount and the amount
 		// requested will be used to pay fees, and generate a change
 		// output with the remaining.
 		overShootAmt := totalSat - amt
 
-		// Based on the estimated size and fee rate, if the excess
+		var changeAmt btcutil.Amount
-		// amount isn't enough to pay fees, then increase the requested
+
-		// coin amount by the estimate required fee, performing another
+		switch {
-		// round of coin selection.
+
-		totalWeight := int64(weightEstimate.Weight())
+		// If the excess amount isn't enough to pay for fees based on
-		requiredFee := feeRate.FeeForWeight(totalWeight)
+		// fee rate and estimated size without using a change output,
-		if overShootAmt < requiredFee {
+		// then increase the requested coin amount by the estimate
-			amtNeeded = amt + requiredFee
+		// required fee without using change, performing another round
+		// of coin selection.
+		case overShootAmt < requiredFeeNoChange:
+			amtNeeded = amt + requiredFeeNoChange
 			continue
+
+		// If sufficient funds were selected to cover the fee required
+		// to include a change output, the remainder will be our change
+		// amount.
+		case overShootAmt > requiredFeeWithChange:
+			changeAmt = overShootAmt - requiredFeeWithChange
+
+		// Otherwise we have selected enough to pay for a tx without a
+		// change output.
+		default:
+			changeAmt = 0
+
 		}
 
-		// If the fee is sufficient, then calculate the size of the
+		if changeAmt < dustLimit {
-		// change output.
+			changeAmt = 0
-		changeAmt := overShootAmt - requiredFee
+		}
+
+		// Sanity check the resulting output values to make sure we
+		// don't burn a great part to fees.
+		totalOut := amt + changeAmt
+		err = sanityCheckFee(totalOut, totalSat-totalOut)
+		if err != nil {
+			return nil, 0, err
+		}
 
 		return selectedUtxos, changeAmt, nil
 	}
@@ -134,37 +204,17 @@ func CoinSelectSubtractFees(feeRate chainfee.SatPerKWeight, amt,
 		return nil, 0, 0, err
 	}
 
-	var weightEstimate input.TxWeightEstimator
+	// Obtain fee estimates both with and without using a change
-	for _, utxo := range selectedUtxos {
-		switch {
-
-		case txscript.IsPayToWitnessPubKeyHash(utxo.PkScript):
-			weightEstimate.AddP2WKHInput()
-
-		case txscript.IsPayToScriptHash(utxo.PkScript):
-			weightEstimate.AddNestedP2WKHInput()
-
-		default:
-			return nil, 0, 0, fmt.Errorf("unsupported address "+
-				"type: %x", utxo.PkScript)
-		}
-	}
-
-	// Channel funding multisig output is P2WSH.
-	weightEstimate.AddP2WSHOutput()
-
-	// At this point we've got two possibilities, either create a
-	// change output, or not. We'll first try without creating a
-	// change output.
-	//
-	// Estimate the fee required for a transaction without a change
 	// output.
-	totalWeight := int64(weightEstimate.Weight())
+	requiredFeeNoChange, requiredFeeWithChange, err := calculateFees(
-	requiredFee := feeRate.FeeForWeight(totalWeight)
+		selectedUtxos, feeRate)
+	if err != nil {
+		return nil, 0, 0, err
+	}
 
 	// For a transaction without a change output, we'll let everything go
 	// to our multi-sig output after subtracting fees.
-	outputAmt := totalSat - requiredFee
+	outputAmt := totalSat - requiredFeeNoChange
 	changeAmt := btcutil.Amount(0)
 
 	// If the the output is too small after subtracting the fee, the coin
@@ -172,24 +222,13 @@ func CoinSelectSubtractFees(feeRate chainfee.SatPerKWeight, amt,
 	if outputAmt <= dustLimit {
 		return nil, 0, 0, fmt.Errorf("output amount(%v) after "+
 			"subtracting fees(%v) below dust limit(%v)", outputAmt,
-			requiredFee, dustLimit)
+			requiredFeeNoChange, dustLimit)
 	}
 
-	// We were able to create a transaction with no change from the
-	// selected inputs. We'll remember the resulting values for
-	// now, while we try to add a change output. Assume that change output
-	// is a P2WKH output.
-	weightEstimate.AddP2WKHOutput()
-
-	// Now that we have added the change output, redo the fee
-	// estimate.
-	totalWeight = int64(weightEstimate.Weight())
-	requiredFee = feeRate.FeeForWeight(totalWeight)
-
 	// For a transaction with a change output, everything we don't spend
 	// will go to change.
+	newOutput := amt - requiredFeeWithChange
 	newChange := totalSat - amt
-	newOutput := amt - requiredFee
 
 	// If adding a change output leads to both outputs being above
 	// the dust limit, we'll add the change output. Otherwise we'll
@@ -202,14 +241,9 @@ func CoinSelectSubtractFees(feeRate chainfee.SatPerKWeight, amt,
 	// Sanity check the resulting output values to make sure we
 	// don't burn a great part to fees.
 	totalOut := outputAmt + changeAmt
-	fee := totalSat - totalOut
+	err = sanityCheckFee(totalOut, totalSat-totalOut)
-
+	if err != nil {
-	// Fail if more than 20% goes to fees.
+		return nil, 0, 0, err
-	// TODO(halseth): smarter fee limit. Make configurable or dynamic wrt
-	// total funding size?
-	if fee > totalOut/5 {
-		return nil, 0, 0, fmt.Errorf("fee %v on total output "+
-			"value %v", fee, totalOut)
 	}
 
 	return selectedUtxos, outputAmt, changeAmt, nil

lnwallet/chanfunding/coin_select_test.go

@@ -9,12 +9,21 @@ import (
 	"github.com/btcsuite/btcutil"
 	"github.com/lightningnetwork/lnd/input"
 	"github.com/lightningnetwork/lnd/lnwallet/chainfee"
+	"github.com/stretchr/testify/require"
 )
 
 var (
 	p2wkhScript, _ = hex.DecodeString(
 		"001411034bdcb6ccb7744fdfdeea958a6fb0b415a032",
 	)
+
+	np2wkhScript, _ = hex.DecodeString(
+		"a914f7bd5b8077b9549653dacf96f824af9d931663e687",
+	)
+
+	p2khScript, _ = hex.DecodeString(
+		"76a91411034bdcb6ccb7744fdfdeea958a6fb0b415a03288ac",
+	)
 )
 
 // fundingFee is a helper method that returns the fee estimate used for a tx
@@ -42,6 +51,88 @@ func fundingFee(feeRate chainfee.SatPerKWeight, numInput int, // nolint:unparam
 	return feeRate.FeeForWeight(totalWeight)
 }
 
+// TestCalculateFees tests that the helper function to calculate the fees
+// both with and without applying a change output is done correctly for
+// (N)P2WKH inputs, and should raise an error otherwise.
+func TestCalculateFees(t *testing.T) {
+	t.Parallel()
+
+	const feeRate = chainfee.SatPerKWeight(1000)
+
+	type testCase struct {
+		name  string
+		utxos []Coin
+
+		expectedFeeNoChange   btcutil.Amount
+		expectedFeeWithChange btcutil.Amount
+		expectedErr           error
+	}
+
+	testCases := []testCase{
+		{
+			name: "one P2WKH input",
+			utxos: []Coin{
+				{
+					TxOut: wire.TxOut{
+						PkScript: p2wkhScript,
+						Value:    1,
+					},
+				},
+			},
+
+			expectedFeeNoChange:   487,
+			expectedFeeWithChange: 611,
+			expectedErr:           nil,
+		},
+
+		{
+			name: "one NP2WKH input",
+			utxos: []Coin{
+				{
+					TxOut: wire.TxOut{
+						PkScript: np2wkhScript,
+						Value:    1,
+					},
+				},
+			},
+
+			expectedFeeNoChange:   579,
+			expectedFeeWithChange: 703,
+			expectedErr:           nil,
+		},
+
+		{
+			name: "not supported P2KH input",
+			utxos: []Coin{
+				{
+					TxOut: wire.TxOut{
+						PkScript: p2khScript,
+						Value:    1,
+					},
+				},
+			},
+
+			expectedErr: &errUnsupportedInput{p2khScript},
+		},
+	}
+
+	for _, test := range testCases {
+		test := test
+		t.Run(test.name, func(t *testing.T) {
+			feeNoChange, feeWithChange, err := calculateFees(
+				test.utxos, feeRate,
+			)
+			require.Equal(t, test.expectedErr, err)
+
+			// Note: The error-case will have zero values returned
+			// for fees and therefore anyway pass the following
+			// requirements.
+			require.Equal(t, test.expectedFeeNoChange, feeNoChange)
+			require.Equal(t, test.expectedFeeWithChange, feeWithChange)
+		})
+	}
+}
+
 // TestCoinSelect tests that we pick coins adding up to the expected amount
 // when creating a funding transaction, and that the calculated change is the
 // expected amount.
@@ -52,7 +143,7 @@ func TestCoinSelect(t *testing.T) {
 	t.Parallel()
 
 	const feeRate = chainfee.SatPerKWeight(100)
-	const dust = btcutil.Amount(100)
+	const dustLimit = btcutil.Amount(1000)
 
 	type testCase struct {
 		name        string
@@ -106,7 +197,7 @@ func TestCoinSelect(t *testing.T) {
 		{
 			// We have a 1 BTC input, and want to create an output
 			// as big as possible, such that the remaining change
-			// will be dust.
+			// would be dust but instead goes to fees.
 			name: "dust change",
 			coins: []Coin{
 				{
@@ -117,41 +208,53 @@ func TestCoinSelect(t *testing.T) {
 				},
 			},
 			// We tune the output value by subtracting the expected
-			// fee and a small dust amount.
+			// fee and the dustlimit.
-			outputValue: 1*btcutil.SatoshiPerBitcoin - fundingFee(feeRate, 1, true) - dust,
+			outputValue: 1*btcutil.SatoshiPerBitcoin - fundingFee(feeRate, 1, false) - dustLimit,
 
 			expectedInput: []btcutil.Amount{
 				1 * btcutil.SatoshiPerBitcoin,
 			},
 
-			// Change will the dust.
+			// Change must be zero.
-			expectedChange: dust,
+			expectedChange: 0,
 		},
 		{
-			// We have a 1 BTC input, and want to create an output
+			// We got just enough funds to create a change output above the
-			// as big as possible, such that there is nothing left
+			// dust limit.
-			// for change.
+			name: "change right above dustlimit",
-			name: "no change",
 			coins: []Coin{
 				{
 					TxOut: wire.TxOut{
 						PkScript: p2wkhScript,
-						Value:    1 * btcutil.SatoshiPerBitcoin,
+						Value:    int64(fundingFee(feeRate, 1, true) + 2*(dustLimit+1)),
 					},
 				},
 			},
-			// We tune the output value to be the maximum amount
+			// We tune the output value to be just above the dust limit.
-			// possible, leaving just enough for fees.
+			outputValue: dustLimit + 1,
-			outputValue: 1*btcutil.SatoshiPerBitcoin - fundingFee(feeRate, 1, true),
 
 			expectedInput: []btcutil.Amount{
-				1 * btcutil.SatoshiPerBitcoin,
+				fundingFee(feeRate, 1, true) + 2*(dustLimit+1),
 			},
-			// We have just enough left to pay the fee, so there is
+
-			// nothing left for change.
+			// After paying for the fee the change output should be just above
-			// TODO(halseth): currently coinselect estimates fees
+			// the dust limit.
-			// assuming a change output.
+			expectedChange: dustLimit + 1,
-			expectedChange: 0,
+		},
+		{
+			// If more than 20% of funds goes to fees, it should fail.
+			name: "high fee",
+			coins: []Coin{
+				{
+					TxOut: wire.TxOut{
+						PkScript: p2wkhScript,
+						Value:    int64(5 * fundingFee(feeRate, 1, false)),
+					},
+				},
+			},
+			outputValue: 4 * fundingFee(feeRate, 1, false),
+
+			expectErr: true,
 		},
 	}
 
@@ -161,7 +264,7 @@ func TestCoinSelect(t *testing.T) {
 			t.Parallel()
 
 			selected, changeAmt, err := CoinSelect(
-				feeRate, test.outputValue, test.coins,
+				feeRate, test.outputValue, dustLimit, test.coins,
 			)
 			if !test.expectErr && err != nil {
 				t.Fatalf(err.Error())

lnwallet/chanfunding/wallet_assembler.go

@@ -1,6 +1,7 @@
 package chanfunding
 
 import (
+	"fmt"
 	"math"
 
 	"github.com/btcsuite/btcd/btcec"
@@ -289,20 +290,28 @@ func (w *WalletAssembler) ProvisionChannel(r *Request) (Intent, error) {
 		// Otherwise do a normal coin selection where we target a given
 		// funding amount.
 		default:
+			dustLimit := w.cfg.DustLimit
 			localContributionAmt = r.LocalAmt
 			selectedCoins, changeAmt, err = CoinSelect(
-				r.FeeRate, r.LocalAmt, coins,
+				r.FeeRate, r.LocalAmt, dustLimit, coins,
 			)
 			if err != nil {
 				return err
 			}
 		}
 
+		// Sanity check: The addition of the outputs should not lead to the
+		// creation of dust.
+		if changeAmt != 0 && changeAmt <= w.cfg.DustLimit {
+			return fmt.Errorf("change amount(%v) after coin "+
+				"select is below dust limit(%v)", changeAmt,
+				w.cfg.DustLimit)
+		}
+
 		// Record any change output(s) generated as a result of the
-		// coin selection, but only if the addition of the output won't
+		// coin selection.
-		// lead to the creation of dust.
 		var changeOutput *wire.TxOut
-		if changeAmt != 0 && changeAmt > w.cfg.DustLimit {
+		if changeAmt != 0 {
 			changeAddr, err := r.ChangeAddr()
 			if err != nil {
 				return err