chanfunding: refactor fee estimate calculation

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,6 +64,60 @@ 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
@@ -68,34 +134,14 @@ 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 {
+		_, 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.
@@ -105,16 +151,14 @@ func CoinSelect(feeRate chainfee.SatPerKWeight, amt btcutil.Amount,
 		// amount isn't enough to pay fees, then increase the requested
 		// coin amount by the estimate required fee, performing another
 		// round of coin selection.
-		totalWeight := int64(weightEstimate.Weight())
+		if overShootAmt < requiredFeeWithChange {
-		requiredFee := feeRate.FeeForWeight(totalWeight)
+			amtNeeded = amt + requiredFeeWithChange
-		if overShootAmt < requiredFee {
-			amtNeeded = amt + requiredFee
 			continue
 		}
 
 		// If the fee is sufficient, then calculate the size of the
 		// change output.
-		changeAmt := overShootAmt - requiredFee
+		changeAmt := overShootAmt - requiredFeeWithChange
 
 		return selectedUtxos, changeAmt, nil
 	}
@@ -134,37 +178,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 +196,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 +215,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.