lnwallet: use new maxFeeRatio for sanityCheckFee func

Author: GeorgeTsagk

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)

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
 	}

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())

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