From 235e73205de74001615caf2c4c9b48a9b21d5fad Mon Sep 17 00:00:00 2001 From: Bjarne Magnussen Date: Mon, 12 Apr 2021 14:39:32 +0200 Subject: [PATCH] chanfunding: refactor fee estimate calculation --- lnwallet/chanfunding/coin_select.go | 168 ++++++++++++----------- lnwallet/chanfunding/coin_select_test.go | 91 ++++++++++++ 2 files changed, 179 insertions(+), 80 deletions(-) diff --git a/lnwallet/chanfunding/coin_select.go b/lnwallet/chanfunding/coin_select.go index 9a95b74e..dfb55ec6 100644 --- a/lnwallet/chanfunding/coin_select.go +++ b/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 - - for _, utxo := range selectedUtxos { - switch { - - case txscript.IsPayToWitnessPubKeyHash(utxo.PkScript): - weightEstimate.AddP2WKHInput() - - case txscript.IsPayToScriptHash(utxo.PkScript): - weightEstimate.AddNestedP2WKHInput() - - default: - return nil, 0, fmt.Errorf("unsupported address type: %x", - utxo.PkScript) - } + // Obtain fee estimates both with and without using a change + // output. + _, requiredFeeWithChange, err := calculateFees( + selectedUtxos, feeRate, + ) + if err != nil { + return nil, 0, err } - - // 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()) - requiredFee := feeRate.FeeForWeight(totalWeight) - if overShootAmt < requiredFee { - amtNeeded = amt + requiredFee + if overShootAmt < requiredFeeWithChange { + amtNeeded = amt + requiredFeeWithChange 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 - 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 + // Obtain fee estimates both with and without using a change // output. - totalWeight := int64(weightEstimate.Weight()) - requiredFee := feeRate.FeeForWeight(totalWeight) + requiredFeeNoChange, requiredFeeWithChange, err := calculateFees( + 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 - - // 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 nil, 0, 0, fmt.Errorf("fee %v on total output "+ - "value %v", fee, totalOut) + err = sanityCheckFee(totalOut, totalSat-totalOut) + if err != nil { + return nil, 0, 0, err } return selectedUtxos, outputAmt, changeAmt, nil diff --git a/lnwallet/chanfunding/coin_select_test.go b/lnwallet/chanfunding/coin_select_test.go index c26dd756..eb000612 100644 --- a/lnwallet/chanfunding/coin_select_test.go +++ b/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.