Merge pull request #7645 from shaurya947/bitcoind-minmempoolfee

sweep+lnrpc: enforce provided fee rate is no less than relay fee

docs/release-notes/release-notes-0.17.0.md

@@ -31,6 +31,10 @@ package](https://github.com/lightningnetwork/lnd/pull/7356)
 * [SendOutputs](https://github.com/lightningnetwork/lnd/pull/7631) now adheres
   to the anchor channel reserve requirement.
 
+* Enforce provided [fee rate is no less than the relay or minimum mempool
+  fee](https://github.com/lightningnetwork/lnd/pull/7645) when calling
+  `OpenChannel`, `CloseChannel`, `SendCoins`, and `SendMany`.
+
 * The [UpdateNodeAnnouncement](https://github.com/lightningnetwork/lnd/pull/7568)
   API can no longer be used to set/unset protocol features that are defined by 
   LND.  
@@ -101,4 +105,5 @@ unlock or create.
 * Matt Morehouse
 * Michael Street
 * Oliver Gugger
+* Shaurya Arora
 * ziggie1984

lnrpc/rpc_utils.go

@@ -3,10 +3,13 @@ package lnrpc
 import (
 	"encoding/hex"
 	"errors"
+	"fmt"
 	"sort"
 
 	"github.com/btcsuite/btcd/chaincfg/chainhash"
 	"github.com/lightningnetwork/lnd/lnwallet"
+	"github.com/lightningnetwork/lnd/lnwallet/chainfee"
+	"github.com/lightningnetwork/lnd/sweep"
 	"google.golang.org/protobuf/encoding/protojson"
 )
 
@@ -193,3 +196,41 @@ func GetChanPointFundingTxid(chanPoint *ChannelPoint) (*chainhash.Hash, error) {
 
 	return chainhash.NewHash(txid)
 }
+
+// CalculateFeeRate uses either satPerByte or satPerVByte, but not both, from a
+// request to calculate the fee rate. It provides compatibility for the
+// deprecated field, satPerByte. Once the field is safe to be removed, the
+// check can then be deleted.
+func CalculateFeeRate(satPerByte, satPerVByte uint64, targetConf uint32,
+	estimator chainfee.Estimator) (chainfee.SatPerKWeight, error) {
+
+	var feeRate chainfee.SatPerKWeight
+
+	// We only allow using either the deprecated field or the new field.
+	if satPerByte != 0 && satPerVByte != 0 {
+		return feeRate, fmt.Errorf("either SatPerByte or " +
+			"SatPerVByte should be set, but not both")
+	}
+
+	// Default to satPerVByte, and overwrite it if satPerByte is set.
+	satPerKw := chainfee.SatPerKVByte(satPerVByte * 1000).FeePerKWeight()
+	if satPerByte != 0 {
+		satPerKw = chainfee.SatPerKVByte(
+			satPerByte * 1000,
+		).FeePerKWeight()
+	}
+
+	// Based on the passed fee related parameters, we'll determine an
+	// appropriate fee rate for this transaction.
+	feeRate, err := sweep.DetermineFeePerKw(
+		estimator, sweep.FeePreference{
+			ConfTarget: targetConf,
+			FeeRate:    satPerKw,
+		},
+	)
+	if err != nil {
+		return feeRate, err
+	}
+
+	return feeRate, nil
+}

rpcserver.go

@@ -222,44 +222,6 @@ func stringInSlice(a string, slice []string) bool {
 	return false
 }
 
-// calculateFeeRate uses either satPerByte or satPerVByte, but not both, from a
-// request to calculate the fee rate. It provides compatibility for the
-// deprecated field, satPerByte. Once the field is safe to be removed, the
-// check can then be deleted.
-func calculateFeeRate(satPerByte, satPerVByte uint64, targetConf uint32,
-	estimator chainfee.Estimator) (chainfee.SatPerKWeight, error) {
-
-	var feeRate chainfee.SatPerKWeight
-
-	// We only allow using either the deprecated field or the new field.
-	if satPerByte != 0 && satPerVByte != 0 {
-		return feeRate, fmt.Errorf("either SatPerByte or " +
-			"SatPerVByte should be set, but not both")
-	}
-
-	// Default to satPerVByte, and overwrite it if satPerByte is set.
-	satPerKw := chainfee.SatPerKVByte(satPerVByte * 1000).FeePerKWeight()
-	if satPerByte != 0 {
-		satPerKw = chainfee.SatPerKVByte(
-			satPerByte * 1000,
-		).FeePerKWeight()
-	}
-
-	// Based on the passed fee related parameters, we'll determine an
-	// appropriate fee rate for this transaction.
-	feeRate, err := sweep.DetermineFeePerKw(
-		estimator, sweep.FeePreference{
-			ConfTarget: targetConf,
-			FeeRate:    satPerKw,
-		},
-	)
-	if err != nil {
-		return feeRate, err
-	}
-
-	return feeRate, nil
-}
-
 // GetAllPermissions returns all the permissions required to interact with lnd.
 func GetAllPermissions() []bakery.Op {
 	allPerms := make([]bakery.Op, 0)
@@ -1239,7 +1201,7 @@ func (r *rpcServer) SendCoins(ctx context.Context,
 	in *lnrpc.SendCoinsRequest) (*lnrpc.SendCoinsResponse, error) {
 
 	// Calculate an appropriate fee rate for this transaction.
-	feePerKw, err := calculateFeeRate(
+	feePerKw, err := lnrpc.CalculateFeeRate(
 		uint64(in.SatPerByte), in.SatPerVbyte, // nolint:staticcheck
 		uint32(in.TargetConf), r.server.cc.FeeEstimator,
 	)
@@ -1452,7 +1414,7 @@ func (r *rpcServer) SendMany(ctx context.Context,
 	in *lnrpc.SendManyRequest) (*lnrpc.SendManyResponse, error) {
 
 	// Calculate an appropriate fee rate for this transaction.
-	feePerKw, err := calculateFeeRate(
+	feePerKw, err := lnrpc.CalculateFeeRate(
 		uint64(in.SatPerByte), in.SatPerVbyte, // nolint:staticcheck
 		uint32(in.TargetConf), r.server.cc.FeeEstimator,
 	)
@@ -2093,7 +2055,7 @@ func (r *rpcServer) parseOpenChannelReq(in *lnrpc.OpenChannelRequest,
 	}
 
 	// Calculate an appropriate fee rate for this transaction.
-	feeRate, err := calculateFeeRate(
+	feeRate, err := lnrpc.CalculateFeeRate(
 		uint64(in.SatPerByte), in.SatPerVbyte, // nolint:staticcheck
 		uint32(in.TargetConf), r.server.cc.FeeEstimator,
 	)
@@ -2584,7 +2546,7 @@ func (r *rpcServer) CloseChannel(in *lnrpc.CloseChannelRequest,
 		// Based on the passed fee related parameters, we'll determine
 		// an appropriate fee rate for the cooperative closure
 		// transaction.
-		feeRate, err := calculateFeeRate(
+		feeRate, err := lnrpc.CalculateFeeRate(
 			uint64(in.SatPerByte), in.SatPerVbyte, // nolint:staticcheck
 			uint32(in.TargetConf), r.server.cc.FeeEstimator,
 		)

sweep/walletsweep.go

@@ -71,14 +71,29 @@ func DetermineFeePerKw(feeEstimator chainfee.Estimator,
 	// internally.
 	case feePref.FeeRate != 0:
 		feePerKW := feePref.FeeRate
-		if feePerKW < chainfee.FeePerKwFloor {
+
+		// Because the user can specify 1 sat/vByte on the RPC
+		// interface, which corresponds to 250 sat/kw, we need to bump
+		// that to the minimum "safe" fee rate which is 253 sat/kw.
+		if feePerKW == chainfee.AbsoluteFeePerKwFloor {
 			log.Infof("Manual fee rate input of %d sat/kw is "+
 				"too low, using %d sat/kw instead", feePerKW,
 				chainfee.FeePerKwFloor)
-
 			feePerKW = chainfee.FeePerKwFloor
 		}
 
+		// If that bumped fee rate of at least 253 sat/kw is still lower
+		// than the relay fee rate, we return an error to let the user
+		// know. Note that "Relay fee rate" may mean slightly different
+		// things depending on the backend. For bitcoind, it is
+		// effectively max(relay fee, min mempool fee).
+		minFeePerKW := feeEstimator.RelayFeePerKW()
+		if feePerKW < minFeePerKW {
+			return 0, fmt.Errorf("manual fee rate input of %d "+
+				"sat/kw is too low to be accepted into the "+
+				"mempool or relayed to the network", feePerKW)
+		}
+
 		return feePerKW, nil
 
 	// Otherwise, we'll attempt a relaxed confirmation target for the

sweep/walletsweep_test.go

@@ -43,12 +43,20 @@ func TestDetermineFeePerKw(t *testing.T) {
 		// fail determines if this test case should fail or not.
 		fail bool
 	}{
-		// A fee rate below the fee rate floor should output the floor.
+		// A fee rate below the floor should error out.
 		{
 			feePref: FeePreference{
 				FeeRate: chainfee.SatPerKWeight(99),
 			},
-			fee: chainfee.FeePerKwFloor,
+			fail: true,
+		},
+
+		// A fee rate below the relay fee should error out.
+		{
+			feePref: FeePreference{
+				FeeRate: chainfee.SatPerKWeight(299),
+			},
+			fail: true,
 		},
 
 		// A fee rate above the floor, should pass through and return