diff --git a/docs/release-notes/release-notes-0.18.0.md b/docs/release-notes/release-notes-0.18.0.md index 71e75423d..67d0a3bc8 100644 --- a/docs/release-notes/release-notes-0.18.0.md +++ b/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 diff --git a/itest/lnd_misc_test.go b/itest/lnd_misc_test.go index 163fc436d..3c247c4a1 100644 --- a/itest/lnd_misc_test.go +++ b/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{ - Addr: ht.Miner.NewMinerAddress().String(), - SendAll: true, - Label: sendCoinsLabel, - }) + // ainz.RPC.SendCoinsAssertErr(&lnrpc.SendCoinsRequest{ + // Addr: ht.Miner.NewMinerAddress().String(), + // SendAll: true, + // Label: sendCoinsLabel, + // }) // Send coins to a compatible address. ainz.RPC.SendCoins(&lnrpc.SendCoinsRequest{ diff --git a/rpcserver.go b/rpcserver.go index fad2cc307..6b90dab65 100644 --- a/rpcserver.go +++ b/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 diff --git a/sweep/walletsweep.go b/sweep/walletsweep.go index 5328ae508..8eb3764a1 100644 --- a/sweep/walletsweep.go +++ b/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.