Merge pull request #8693 from yyforyongyu/add-default-conf

rpc: add default conf target back

docs/release-notes/release-notes-0.18.0.md

@@ -349,14 +349,6 @@ bitcoin peers' feefilter values into account](https://github.com/lightningnetwor
   add coin selection strategy option to the following on-chain RPC calls
   `EstimateFee`, `SendMany`, `SendCoins`, `BatchOpenChannel`, `SendOutputs`, and `FundPsbt`.
 
-* Previously when callng `SendCoins`, `SendMany`, `OpenChannel` and
-  `CloseChannel` for coop close, it is allowed to specify both an empty
-  `SatPerVbyte` and `TargetConf`, and a default conf target of 6 will be used.
-  This is [no longer allowed](
-  https://github.com/lightningnetwork/lnd/pull/8422) and the caller must
-  specify either `SatPerVbyte` or `TargetConf` so the fee estimator can do a
-  proper fee estimation.
-
 * `BumpFee` has been updated to take advantage of the [new budget-based
   sweeper](https://github.com/lightningnetwork/lnd/pull/8667). The param
   `force` has been deprecated and replaced with a new param `immediate`, and a
@@ -432,6 +424,18 @@ bitcoin peers' feefilter values into account](https://github.com/lightningnetwor
   manage the lifecycle of the inputs.
 
 ## Breaking Changes
+
+* Previously when callng `SendCoins`, `SendMany`, `OpenChannel` and
+  `CloseChannel` for coop close, it is allowed to specify both an empty
+  `SatPerVbyte` and `TargetConf`, and a default conf target of 6 will be used.
+  This will [no longer be
+  allowed](https://github.com/lightningnetwork/lnd/pull/8422) in the next
+  release (v0.19.0) and the caller must specify either `SatPerVbyte` or
+  `TargetConf` so the fee estimator can do a proper fee estimation. For current
+  release, [an error will be
+  logged](https://github.com/lightningnetwork/lnd/pull/8693) when no values are
+  specified.
+
 ## Performance Improvements
 
 * Watchtower client DB migration to massively [improve the start-up 

itest/lnd_misc_test.go

@@ -815,13 +815,18 @@ func testSweepAllCoins(ht *lntest.HarnessTest) {
 		TargetConf: 6,
 	})
 
+	// TODO(yy): we still allow default values to be used when neither conf
+	// target or fee rate is set in 0.18.0. When future release forbidden
+	// this behavior, we should revive the test below, which asserts either
+	// conf target or fee rate is set.
+	//
 	// Send coins to a compatible address without specifying fee rate or
 	// conf target.
-	ainz.RPC.SendCoinsAssertErr(&lnrpc.SendCoinsRequest{
+	// ainz.RPC.SendCoinsAssertErr(&lnrpc.SendCoinsRequest{
-		Addr:    ht.Miner.NewMinerAddress().String(),
+	// 	Addr:    ht.Miner.NewMinerAddress().String(),
-		SendAll: true,
+	// 	SendAll: true,
-		Label:   sendCoinsLabel,
+	// 	Label:   sendCoinsLabel,
-	})
+	// })
 
 	// Send coins to a compatible address.
 	ainz.RPC.SendCoins(&lnrpc.SendCoinsRequest{

rpcserver.go

@@ -85,6 +85,13 @@ import (
 	"gopkg.in/macaroon-bakery.v2/bakery"
 )
 
+const (
+	// defaultNumBlocksEstimate is the number of blocks that we fall back
+	// to issuing an estimate for if a fee pre fence doesn't specify an
+	// explicit conf target or fee rate.
+	defaultNumBlocksEstimate = 6
+)
+
 var (
 	// readPermissions is a slice of all entities that allow read
 	// permissions for authorization purposes, all lowercase.
@@ -1239,15 +1246,46 @@ func (r *rpcServer) EstimateFee(ctx context.Context,
 	return resp, nil
 }
 
+// maybeUseDefaultConf makes sure that when the user doesn't set either the fee
+// rate or conf target, the default conf target is used.
+func maybeUseDefaultConf(satPerByte int64, satPerVByte uint64,
+	targetConf uint32) uint32 {
+
+	// If the fee rate is set, there's no need to use the default conf
+	// target. In this case, we just return the targetConf from the
+	// request.
+	if satPerByte != 0 || satPerVByte != 0 {
+		return targetConf
+	}
+
+	// Return the user specified conf target if set.
+	if targetConf != 0 {
+		return targetConf
+	}
+
+	// If the fee rate is not set, yet the conf target is zero, the default
+	// 6 will be returned.
+	rpcsLog.Errorf("Expected either 'sat_per_vbyte' or 'conf_target' to " +
+		"be set, using default conf of 6 instead")
+
+	return defaultNumBlocksEstimate
+}
+
 // SendCoins executes a request to send coins to a particular address. Unlike
 // SendMany, this RPC call only allows creating a single output at a time.
 func (r *rpcServer) SendCoins(ctx context.Context,
 	in *lnrpc.SendCoinsRequest) (*lnrpc.SendCoinsResponse, error) {
 
+	// Keep the old behavior prior to 0.18.0 - when the user doesn't set
+	// fee rate or conf target, the default conf target of 6 is used.
+	targetConf := maybeUseDefaultConf(
+		in.SatPerByte, in.SatPerVbyte, uint32(in.TargetConf),
+	)
+
 	// Calculate an appropriate fee rate for this transaction.
 	feePerKw, err := lnrpc.CalculateFeeRate(
 		uint64(in.SatPerByte), in.SatPerVbyte, // nolint:staticcheck
-		uint32(in.TargetConf), r.server.cc.FeeEstimator,
+		targetConf, r.server.cc.FeeEstimator,
 	)
 	if err != nil {
 		return nil, err
@@ -1465,10 +1503,16 @@ func (r *rpcServer) SendCoins(ctx context.Context,
 func (r *rpcServer) SendMany(ctx context.Context,
 	in *lnrpc.SendManyRequest) (*lnrpc.SendManyResponse, error) {
 
+	// Keep the old behavior prior to 0.18.0 - when the user doesn't set
+	// fee rate or conf target, the default conf target of 6 is used.
+	targetConf := maybeUseDefaultConf(
+		in.SatPerByte, in.SatPerVbyte, uint32(in.TargetConf),
+	)
+
 	// Calculate an appropriate fee rate for this transaction.
 	feePerKw, err := lnrpc.CalculateFeeRate(
 		uint64(in.SatPerByte), in.SatPerVbyte, // nolint:staticcheck
-		uint32(in.TargetConf), r.server.cc.FeeEstimator,
+		targetConf, r.server.cc.FeeEstimator,
 	)
 	if err != nil {
 		return nil, err
@@ -2129,10 +2173,17 @@ func (r *rpcServer) parseOpenChannelReq(in *lnrpc.OpenChannelRequest,
 
 	// Skip estimating fee rate for PSBT funding.
 	if in.FundingShim == nil || in.FundingShim.GetPsbtShim() == nil {
+		// Keep the old behavior prior to 0.18.0 - when the user
+		// doesn't set fee rate or conf target, the default conf target
+		// of 6 is used.
+		targetConf := maybeUseDefaultConf(
+			in.SatPerByte, in.SatPerVbyte, uint32(in.TargetConf),
+		)
+
 		// Calculate an appropriate fee rate for this transaction.
 		feeRate, err = lnrpc.CalculateFeeRate(
 			uint64(in.SatPerByte), in.SatPerVbyte,
-			uint32(in.TargetConf), r.server.cc.FeeEstimator,
+			targetConf, r.server.cc.FeeEstimator,
 		)
 		if err != nil {
 			return nil, err
@@ -2680,12 +2731,19 @@ func (r *rpcServer) CloseChannel(in *lnrpc.CloseChannelRequest,
 				"is offline (try force closing it instead): %v", err)
 		}
 
+		// Keep the old behavior prior to 0.18.0 - when the user
+		// doesn't set fee rate or conf target, the default conf target
+		// of 6 is used.
+		targetConf := maybeUseDefaultConf(
+			in.SatPerByte, in.SatPerVbyte, uint32(in.TargetConf),
+		)
+
 		// Based on the passed fee related parameters, we'll determine
 		// an appropriate fee rate for the cooperative closure
 		// transaction.
 		feeRate, err := lnrpc.CalculateFeeRate(
 			uint64(in.SatPerByte), in.SatPerVbyte, // nolint:staticcheck
-			uint32(in.TargetConf), r.server.cc.FeeEstimator,
+			targetConf, r.server.cc.FeeEstimator,
 		)
 		if err != nil {
 			return err

sweep/walletsweep.go

@@ -16,13 +16,6 @@ import (
 	"github.com/lightningnetwork/lnd/lnwallet/chanfunding"
 )
 
-const (
-	// defaultNumBlocksEstimate is the number of blocks that we fall back
-	// to issuing an estimate for if a fee pre fence doesn't specify an
-	// explicit conf target or fee rate.
-	defaultNumBlocksEstimate = 6
-)
-
 var (
 	// ErrNoFeePreference is returned when we attempt to satisfy a sweep
 	// request from a client whom did not specify a fee preference.