diff --git a/lnwallet/chanfunding/coin_select.go b/lnwallet/chanfunding/coin_select.go index 9a95b74e0..74a901ba8 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,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 - - 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. + requiredFeeNoChange, 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. overShootAmt := totalSat - amt - // Based on the estimated size and fee rate, if the excess - // 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 + var changeAmt btcutil.Amount + + switch { + + // If the excess amount isn't enough to pay for fees based on + // fee rate and estimated size without using a change output, + // then increase the requested coin amount by the estimate + // 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 - // change output. - changeAmt := overShootAmt - requiredFee + if changeAmt < dustLimit { + changeAmt = 0 + } + + // 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 - 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 +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 - - // 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 c26dd7567..83c70dec5 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. @@ -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. - outputValue: 1*btcutil.SatoshiPerBitcoin - fundingFee(feeRate, 1, true) - dust, + // fee and the dustlimit. + outputValue: 1*btcutil.SatoshiPerBitcoin - fundingFee(feeRate, 1, false) - dustLimit, expectedInput: []btcutil.Amount{ 1 * btcutil.SatoshiPerBitcoin, }, - // Change will the dust. - expectedChange: dust, + // Change must be zero. + expectedChange: 0, }, { - // We have a 1 BTC input, and want to create an output - // as big as possible, such that there is nothing left - // for change. - name: "no change", + // We got just enough funds to create a change output above the + // dust limit. + name: "change right above dustlimit", 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 - // possible, leaving just enough for fees. - outputValue: 1*btcutil.SatoshiPerBitcoin - fundingFee(feeRate, 1, true), + // We tune the output value to be just above the dust limit. + outputValue: dustLimit + 1, 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. - // TODO(halseth): currently coinselect estimates fees - // assuming a change output. - expectedChange: 0, + + // After paying for the fee the change output should be just above + // the dust limit. + expectedChange: dustLimit + 1, + }, + { + // 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()) diff --git a/lnwallet/chanfunding/wallet_assembler.go b/lnwallet/chanfunding/wallet_assembler.go index 9cdf9b10b..ee335f324 100644 --- a/lnwallet/chanfunding/wallet_assembler.go +++ b/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 - // lead to the creation of dust. + // coin selection. var changeOutput *wire.TxOut - if changeAmt != 0 && changeAmt > w.cfg.DustLimit { + if changeAmt != 0 { changeAddr, err := r.ChangeAddr() if err != nil { return err