diff --git a/lnwallet/wallet.go b/lnwallet/wallet.go index 025977f0..bd9f279d 100644 --- a/lnwallet/wallet.go +++ b/lnwallet/wallet.go @@ -1455,7 +1455,7 @@ func coinSelect(feeRate SatPerKWeight, amt btcutil.Amount, case NestedWitnessPubKey: weightEstimate.AddNestedP2WKHInput() default: - return nil, 0, fmt.Errorf("Unsupported address type: %v", + return nil, 0, fmt.Errorf("unsupported address type: %v", utxo.AddressType) } } @@ -1494,3 +1494,95 @@ func coinSelect(feeRate SatPerKWeight, amt btcutil.Amount, return selectedUtxos, changeAmt, nil } } + +// coinSelectSubtractFees attempts to select coins such that we'll spend up to +// amt in total after fees, adhering to the specified fee rate. The selected +// coins, the final output and change values are returned. +func coinSelectSubtractFees(feeRate SatPerKWeight, amt, + dustLimit btcutil.Amount, coins []*Utxo) ([]*Utxo, btcutil.Amount, + btcutil.Amount, error) { + + // First perform an initial round of coin selection to estimate + // the required fee. + totalSat, selectedUtxos, err := selectInputs(amt, coins) + if err != nil { + return nil, 0, 0, err + } + + var weightEstimate input.TxWeightEstimator + for _, utxo := range selectedUtxos { + switch utxo.AddressType { + case WitnessPubKey: + weightEstimate.AddP2WKHInput() + case NestedWitnessPubKey: + weightEstimate.AddNestedP2WKHInput() + default: + return nil, 0, 0, fmt.Errorf("unsupported "+ + "address type: %v", utxo.AddressType) + } + } + + // 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()) + requiredFee := feeRate.FeeForWeight(totalWeight) + + // For a transaction without a change output, we'll let everything go + // to our multi-sig output after subtracting fees. + outputAmt := totalSat - requiredFee + changeAmt := btcutil.Amount(0) + + // If the the output is too small after subtracting the fee, the coin + // selection cannot be performed with an amount this small. + if outputAmt <= dustLimit { + return nil, 0, 0, fmt.Errorf("output amount(%v) after "+ + "subtracting fees(%v) below dust limit(%v)", outputAmt, + requiredFee, 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. + 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 + // go with the no change tx we originally found. + if newChange > dustLimit && newOutput > dustLimit { + outputAmt = newOutput + changeAmt = newChange + } + + // 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) + } + + return selectedUtxos, outputAmt, changeAmt, nil +} diff --git a/lnwallet/wallet_test.go b/lnwallet/wallet_test.go index 2c2a21cc..f9a6c0d3 100644 --- a/lnwallet/wallet_test.go +++ b/lnwallet/wallet_test.go @@ -178,3 +178,173 @@ func TestCoinSelect(t *testing.T) { }) } } + +// TestCoinSelectSubtractFees tests that we pick coins adding up to the +// expected amount when creating a funding transaction, and that a change +// output is created only when necessary. +func TestCoinSelectSubtractFees(t *testing.T) { + t.Parallel() + + const feeRate = SatPerKWeight(100) + const dustLimit = btcutil.Amount(1000) + const dust = btcutil.Amount(100) + + type testCase struct { + name string + spendValue btcutil.Amount + coins []*Utxo + + expectedInput []btcutil.Amount + expectedFundingAmt btcutil.Amount + expectedChange btcutil.Amount + expectErr bool + } + + testCases := []testCase{ + { + // We have 1.0 BTC available, spend them all. This + // should lead to a funding TX with one output, the + // rest goes to fees. + name: "spend all", + coins: []*Utxo{ + { + AddressType: WitnessPubKey, + Value: 1 * btcutil.SatoshiPerBitcoin, + }, + }, + spendValue: 1 * btcutil.SatoshiPerBitcoin, + + // The one and only input will be selected. + expectedInput: []btcutil.Amount{ + 1 * btcutil.SatoshiPerBitcoin, + }, + expectedFundingAmt: 1*btcutil.SatoshiPerBitcoin - fundingFee(feeRate, 1, false), + expectedChange: 0, + }, + { + // The total funds available is below the dust limit + // after paying fees. + name: "dust output", + coins: []*Utxo{ + { + AddressType: WitnessPubKey, + Value: fundingFee(feeRate, 1, false) + dust, + }, + }, + spendValue: fundingFee(feeRate, 1, false) + dust, + + expectErr: true, + }, + { + // After subtracting fees, the resulting change output + // is below the dust limit. The remainder should go + // towards the funding output. + name: "dust change", + coins: []*Utxo{ + { + AddressType: WitnessPubKey, + Value: 1 * btcutil.SatoshiPerBitcoin, + }, + }, + spendValue: 1*btcutil.SatoshiPerBitcoin - dust, + + expectedInput: []btcutil.Amount{ + 1 * btcutil.SatoshiPerBitcoin, + }, + expectedFundingAmt: 1*btcutil.SatoshiPerBitcoin - fundingFee(feeRate, 1, false), + expectedChange: 0, + }, + { + // We got just enough funds to create an output above the dust limit. + name: "output right above dustlimit", + coins: []*Utxo{ + { + AddressType: WitnessPubKey, + Value: fundingFee(feeRate, 1, false) + dustLimit + 1, + }, + }, + spendValue: fundingFee(feeRate, 1, false) + dustLimit + 1, + + expectedInput: []btcutil.Amount{ + fundingFee(feeRate, 1, false) + dustLimit + 1, + }, + expectedFundingAmt: dustLimit + 1, + expectedChange: 0, + }, + { + // Amount left is below dust limit after paying fee for + // a change output, resulting in a no-change tx. + name: "no amount to pay fee for change", + coins: []*Utxo{ + { + AddressType: WitnessPubKey, + Value: fundingFee(feeRate, 1, false) + 2*(dustLimit+1), + }, + }, + spendValue: fundingFee(feeRate, 1, false) + dustLimit + 1, + + expectedInput: []btcutil.Amount{ + fundingFee(feeRate, 1, false) + 2*(dustLimit+1), + }, + expectedFundingAmt: 2 * (dustLimit + 1), + expectedChange: 0, + }, + { + // If more than 20% of funds goes to fees, it should fail. + name: "high fee", + coins: []*Utxo{ + { + AddressType: WitnessPubKey, + Value: 5 * fundingFee(feeRate, 1, false), + }, + }, + spendValue: 5 * fundingFee(feeRate, 1, false), + + expectErr: true, + }, + } + + for _, test := range testCases { + t.Run(test.name, func(t *testing.T) { + selected, localFundingAmt, changeAmt, err := coinSelectSubtractFees( + feeRate, test.spendValue, dustLimit, test.coins, + ) + if !test.expectErr && err != nil { + t.Fatalf(err.Error()) + } + + if test.expectErr && err == nil { + t.Fatalf("expected error") + } + + // If we got an expected error, there is nothing more to test. + if test.expectErr { + return + } + + // Check that the selected inputs match what we expect. + if len(selected) != len(test.expectedInput) { + t.Fatalf("expected %v inputs, got %v", + len(test.expectedInput), len(selected)) + } + + for i, coin := range selected { + if coin.Value != test.expectedInput[i] { + t.Fatalf("expected input %v to have value %v, "+ + "had %v", i, test.expectedInput[i], + coin.Value) + } + } + + // Assert we got the expected change amount. + if localFundingAmt != test.expectedFundingAmt { + t.Fatalf("expected %v local funding amt, got %v", + test.expectedFundingAmt, localFundingAmt) + } + if changeAmt != test.expectedChange { + t.Fatalf("expected %v change amt, got %v", + test.expectedChange, changeAmt) + } + }) + } +}