diff --git a/lnrpc/walletrpc/walletkit_server.go b/lnrpc/walletrpc/walletkit_server.go index ff506de40..04aad75d1 100644 --- a/lnrpc/walletrpc/walletkit_server.go +++ b/lnrpc/walletrpc/walletkit_server.go @@ -1921,7 +1921,7 @@ func (w *WalletKit) fundPsbtCoinSelect(account string, changeIndex int32, changeAmt, needMore, err := chanfunding.CalculateChangeAmount( inputSum, outputSum, packetFeeNoChange, - packetFeeWithChange, changeDustLimit, changeType, + packetFeeWithChange, changeDustLimit, changeType, 0, ) if err != nil { return nil, fmt.Errorf("error calculating change "+ @@ -1978,7 +1978,7 @@ func (w *WalletKit) fundPsbtCoinSelect(account string, changeIndex int32, selectedCoins, changeAmount, err := chanfunding.CoinSelect( feeRate, fundingAmount, changeDustLimit, coins, - strategy, estimator, changeType, + strategy, estimator, changeType, 0, ) if err != nil { return fmt.Errorf("error selecting coins: %w", err) diff --git a/lnwallet/chanfunding/coin_select.go b/lnwallet/chanfunding/coin_select.go index e2c85c9d3..26fe9b92b 100644 --- a/lnwallet/chanfunding/coin_select.go +++ b/lnwallet/chanfunding/coin_select.go @@ -58,6 +58,10 @@ const ( // must assert that the output value of the selected existing output // already is above dust when using this change address type. ExistingChangeAddress ChangeAddressType = 2 + + // DefaultMaxFeeRatio is the default fee to total amount of outputs + // ratio that is used to sanity check the fees of a transaction. + DefaultMaxFeeRatio float64 = 0.2 ) // selectInputs selects a slice of inputs necessary to meet the specified @@ -143,16 +147,25 @@ func calculateFees(utxos []wallet.Coin, feeRate chainfee.SatPerKWeight, 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) exceeds 20%% of total output (%v)", - fee, totalOut) +// sanityCheckFee checks if the specified fee amounts to what the provided ratio +// allows. +func sanityCheckFee(totalOut, fee btcutil.Amount, maxFeeRatio float64) error { + // Sanity check the maxFeeRatio itself. + if maxFeeRatio <= 0.00 || maxFeeRatio > 1.00 { + return fmt.Errorf("maxFeeRatio must be between 0.00 and 1.00 "+ + "got %.2f", maxFeeRatio) } + + maxFee := btcutil.Amount(float64(totalOut) * maxFeeRatio) + + // Check that the fees do not exceed the max allowed value. + if fee > maxFee { + return fmt.Errorf("fee %v exceeds max fee (%v) on total "+ + "output value %v with max fee ratio of %.2f", fee, + maxFee, totalOut, maxFeeRatio) + } + + // All checks passed, we return nil to signal that the fees are valid. return nil } @@ -163,7 +176,8 @@ func sanityCheckFee(totalOut, fee btcutil.Amount) error { func CoinSelect(feeRate chainfee.SatPerKWeight, amt, dustLimit btcutil.Amount, coins []wallet.Coin, strategy wallet.CoinSelectionStrategy, existingWeight input.TxWeightEstimator, - changeType ChangeAddressType) ([]wallet.Coin, btcutil.Amount, error) { + changeType ChangeAddressType, maxFeeRatio float64) ([]wallet.Coin, + btcutil.Amount, error) { amtNeeded := amt for { @@ -188,6 +202,7 @@ func CoinSelect(feeRate chainfee.SatPerKWeight, amt, dustLimit btcutil.Amount, changeAmount, newAmtNeeded, err := CalculateChangeAmount( totalSat, amt, requiredFeeNoChange, requiredFeeWithChange, dustLimit, changeType, + maxFeeRatio, ) if err != nil { return nil, 0, err @@ -216,7 +231,8 @@ func CoinSelect(feeRate chainfee.SatPerKWeight, amt, dustLimit btcutil.Amount, // fees and that more coins need to be selected. func CalculateChangeAmount(totalInputAmt, requiredAmt, requiredFeeNoChange, requiredFeeWithChange, dustLimit btcutil.Amount, - changeType ChangeAddressType) (btcutil.Amount, btcutil.Amount, error) { + changeType ChangeAddressType, maxFeeRatio float64) (btcutil.Amount, + btcutil.Amount, error) { // This is just a sanity check to make sure the function is used // correctly. @@ -266,7 +282,8 @@ func CalculateChangeAmount(totalInputAmt, requiredAmt, requiredFeeNoChange, // Sanity check the resulting output values to make sure we // don't burn a great part to fees. totalOut := requiredAmt + changeAmt - err := sanityCheckFee(totalOut, totalInputAmt-totalOut) + + err := sanityCheckFee(totalOut, totalInputAmt-totalOut, maxFeeRatio) if err != nil { return 0, 0, err } @@ -280,9 +297,9 @@ func CalculateChangeAmount(totalInputAmt, requiredAmt, requiredFeeNoChange, func CoinSelectSubtractFees(feeRate chainfee.SatPerKWeight, amt, dustLimit btcutil.Amount, coins []wallet.Coin, strategy wallet.CoinSelectionStrategy, - existingWeight input.TxWeightEstimator, - changeType ChangeAddressType) ([]wallet.Coin, btcutil.Amount, - btcutil.Amount, error) { + existingWeight input.TxWeightEstimator, changeType ChangeAddressType, + maxFeeRatio float64) ([]wallet.Coin, btcutil.Amount, btcutil.Amount, + error) { // First perform an initial round of coin selection to estimate // the required fee. @@ -331,7 +348,7 @@ 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 - err = sanityCheckFee(totalOut, totalSat-totalOut) + err = sanityCheckFee(totalOut, totalSat-totalOut, maxFeeRatio) if err != nil { return nil, 0, 0, err } @@ -347,8 +364,8 @@ func CoinSelectUpToAmount(feeRate chainfee.SatPerKWeight, minAmount, maxAmount, reserved, dustLimit btcutil.Amount, coins []wallet.Coin, strategy wallet.CoinSelectionStrategy, existingWeight input.TxWeightEstimator, - changeType ChangeAddressType) ([]wallet.Coin, btcutil.Amount, - btcutil.Amount, error) { + changeType ChangeAddressType, maxFeeRatio float64) ([]wallet.Coin, + btcutil.Amount, btcutil.Amount, error) { var ( // selectSubtractFee is tracking if our coin selection was @@ -369,7 +386,7 @@ func CoinSelectUpToAmount(feeRate chainfee.SatPerKWeight, minAmount, maxAmount, // maxAmount with or without a change output that covers the miner fee. selected, changeAmt, err := CoinSelect( feeRate, maxAmount, dustLimit, coins, strategy, existingWeight, - changeType, + changeType, maxFeeRatio, ) var errInsufficientFunds *ErrInsufficientFunds @@ -412,7 +429,7 @@ func CoinSelectUpToAmount(feeRate chainfee.SatPerKWeight, minAmount, maxAmount, if selectSubtractFee { selected, outputAmount, changeAmt, err = CoinSelectSubtractFees( feeRate, totalBalance-reserved, dustLimit, coins, - strategy, existingWeight, changeType, + strategy, existingWeight, changeType, maxFeeRatio, ) if err != nil { return nil, 0, 0, err @@ -430,7 +447,7 @@ func CoinSelectUpToAmount(feeRate chainfee.SatPerKWeight, minAmount, maxAmount, return sum } - err = sanityCheckFee(totalOut, sum(selected)-totalOut) + err = sanityCheckFee(totalOut, sum(selected)-totalOut, maxFeeRatio) if err != nil { return nil, 0, 0, err } diff --git a/lnwallet/chanfunding/coin_select_test.go b/lnwallet/chanfunding/coin_select_test.go index 3d09668b4..8242bc874 100644 --- a/lnwallet/chanfunding/coin_select_test.go +++ b/lnwallet/chanfunding/coin_select_test.go @@ -316,7 +316,7 @@ func TestCoinSelect(t *testing.T) { selected, changeAmt, err := CoinSelect( feeRate, test.outputValue, dustLimit, test.coins, wallet.CoinSelectionLargest, - fundingOutputEstimate, test.changeType, + fundingOutputEstimate, test.changeType, 0, ) if test.expectErr { @@ -428,7 +428,7 @@ func TestCalculateChangeAmount(t *testing.T) { changeAmt, needMore, err := CalculateChangeAmount( tc.totalInputAmt, tc.requiredAmt, tc.feeNoChange, tc.feeWithChange, tc.dustLimit, - tc.changeType, + tc.changeType, 0, ) if tc.expectErr != "" { @@ -627,7 +627,7 @@ func TestCoinSelectSubtractFees(t *testing.T) { feeRate, test.spendValue, dustLimit, test.coins, wallet.CoinSelectionLargest, fundingOutputEstimate, - defaultChanFundingChangeType, + defaultChanFundingChangeType, 0, ) if err != nil { switch { @@ -872,7 +872,7 @@ func TestCoinSelectUpToAmount(t *testing.T) { test.reserved, dustLimit, test.coins, wallet.CoinSelectionLargest, fundingOutputEstimate, - defaultChanFundingChangeType, + defaultChanFundingChangeType, 0, ) if len(test.expectErr) == 0 && err != nil { t.Fatal(err.Error()) diff --git a/lnwallet/chanfunding/wallet_assembler.go b/lnwallet/chanfunding/wallet_assembler.go index 3d62649cb..ce2b406c6 100644 --- a/lnwallet/chanfunding/wallet_assembler.go +++ b/lnwallet/chanfunding/wallet_assembler.go @@ -436,6 +436,7 @@ func (w *WalletAssembler) ProvisionChannel(r *Request) (Intent, error) { reserve, w.cfg.DustLimit, coins, w.cfg.CoinSelectionStrategy, fundingOutputWeight, changeType, + DefaultMaxFeeRatio, ) if err != nil { return err @@ -471,6 +472,7 @@ func (w *WalletAssembler) ProvisionChannel(r *Request) (Intent, error) { r.FeeRate, r.LocalAmt, dustLimit, coins, w.cfg.CoinSelectionStrategy, fundingOutputWeight, changeType, + DefaultMaxFeeRatio, ) if err != nil { return err @@ -485,6 +487,7 @@ func (w *WalletAssembler) ProvisionChannel(r *Request) (Intent, error) { r.FeeRate, r.LocalAmt, dustLimit, coins, w.cfg.CoinSelectionStrategy, fundingOutputWeight, changeType, + DefaultMaxFeeRatio, ) if err != nil { return err