From 2292f3fbd0bc80559f7b90401c8df8c16472172d Mon Sep 17 00:00:00 2001 From: shaurya947 Date: Sat, 22 Apr 2023 21:33:15 -0400 Subject: [PATCH 1/4] sweep: use relay fee rate as floor for specified fee rate This ensures that for transactions where a fee rate is specified (instead of a confirmation target), lnd doesn't accept transactions which would be ultimately ignored by the underlying chain's RPC. --- sweep/walletsweep.go | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/sweep/walletsweep.go b/sweep/walletsweep.go index 6044b85f8..f806d3276 100644 --- a/sweep/walletsweep.go +++ b/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 From 25097f09931ce127515a65f588e765247719f34a Mon Sep 17 00:00:00 2001 From: shaurya947 Date: Thu, 27 Apr 2023 18:51:55 -0400 Subject: [PATCH 2/4] sweep: update fee estimator fee-below-floor test case Handle error in test case, as well as add new test case for below relay fee input. --- sweep/walletsweep_test.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/sweep/walletsweep_test.go b/sweep/walletsweep_test.go index 29800bb57..f2196fc89 100644 --- a/sweep/walletsweep_test.go +++ b/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 From 85ab96cd0937483c7a0f7f8aae16ea7cfdfa357b Mon Sep 17 00:00:00 2001 From: shaurya947 Date: Thu, 27 Apr 2023 19:33:21 -0400 Subject: [PATCH 3/4] lnrpc: move CalculateFeeRate from rpcserver.go to rpc_utils.go This refactor aims to house the CalculateFeeRate function in a more "shareable" location. It used to be a non-exported function inside of rpcserver.go. However, other routines, such as FundPsbt inside of walletkit_server.go, could also rely on the same fee calculation functionality. So we move the function to rpc_utils.go and export it, and we will reuse it in the FundPsbt workflow in a future PR. --- lnrpc/rpc_utils.go | 41 +++++++++++++++++++++++++++++++++++++++++ rpcserver.go | 46 ++++------------------------------------------ 2 files changed, 45 insertions(+), 42 deletions(-) diff --git a/lnrpc/rpc_utils.go b/lnrpc/rpc_utils.go index 32899c506..9d9dea320 100644 --- a/lnrpc/rpc_utils.go +++ b/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 +} diff --git a/rpcserver.go b/rpcserver.go index 952b769ec..96e11a288 100644 --- a/rpcserver.go +++ b/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, ) @@ -2574,7 +2536,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, ) From e2bd34b72c2a5fdd7ebb0fec2bb572fb196fc631 Mon Sep 17 00:00:00 2001 From: shaurya947 Date: Thu, 27 Apr 2023 19:47:18 -0400 Subject: [PATCH 4/4] docs: update release notes for 0.17 --- docs/release-notes/release-notes-0.17.0.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/release-notes/release-notes-0.17.0.md b/docs/release-notes/release-notes-0.17.0.md index 00132fdc0..efcef3018 100644 --- a/docs/release-notes/release-notes-0.17.0.md +++ b/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. @@ -76,4 +80,5 @@ unlock or create. * Matt Morehouse * Michael Street * Oliver Gugger +* Shaurya Arora * ziggie1984