From 6312fa116e56ad63c0dc6791e6e02c01eb878da7 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Wed, 12 Jul 2023 21:22:20 +0800 Subject: [PATCH 1/8] sweep: lower the max allowed fee rate for sweeping inputs This commit changes the DefaultMaxFeeRate from 10,120 sat/vb to 1,000 sat/vb. --- sweep/sweeper.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sweep/sweeper.go b/sweep/sweeper.go index 4f53d2098d8..4ec4154366b 100644 --- a/sweep/sweeper.go +++ b/sweep/sweeper.go @@ -22,9 +22,9 @@ import ( const ( // DefaultMaxFeeRate is the default maximum fee rate allowed within the - // UtxoSweeper. The current value is equivalent to a fee rate of 10,000 + // UtxoSweeper. The current value is equivalent to a fee rate of 1,000 // sat/vbyte. - DefaultMaxFeeRate = chainfee.FeePerKwFloor * 1e4 + DefaultMaxFeeRate = chainfee.AbsoluteFeePerKwFloor * 1e3 // DefaultFeeRateBucketSize is the default size of fee rate buckets // we'll use when clustering inputs into buckets with similar fee rates From 62eb60ae5dfd3e2f947f9043f0584b36103d4ff8 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Wed, 26 Jul 2023 10:48:00 +0200 Subject: [PATCH 2/8] chainfee: add new unit `SatPerVByte` --- lnwallet/chainfee/rates.go | 18 ++++++++++++++++++ lnwallet/chainfee/rates_test.go | 22 ++++++++++++++++++++++ 2 files changed, 40 insertions(+) create mode 100644 lnwallet/chainfee/rates_test.go diff --git a/lnwallet/chainfee/rates.go b/lnwallet/chainfee/rates.go index f9b606f7497..34da5537449 100644 --- a/lnwallet/chainfee/rates.go +++ b/lnwallet/chainfee/rates.go @@ -18,6 +18,24 @@ const ( AbsoluteFeePerKwFloor SatPerKWeight = 250 ) +// SatPerVByte represents a fee rate in sat/vbyte. +type SatPerVByte btcutil.Amount + +// FeePerKWeight converts the current fee rate from sat/vb to sat/kw. +func (s SatPerVByte) FeePerKWeight() SatPerKWeight { + return SatPerKWeight(s * 1000 / blockchain.WitnessScaleFactor) +} + +// FeePerKVByte converts the current fee rate from sat/vb to sat/kvb. +func (s SatPerVByte) FeePerKVByte() SatPerKVByte { + return SatPerKVByte(s * 1000) +} + +// String returns a human-readable string of the fee rate. +func (s SatPerVByte) String() string { + return fmt.Sprintf("%v sat/vb", int64(s)) +} + // SatPerKVByte represents a fee rate in sat/kb. type SatPerKVByte btcutil.Amount diff --git a/lnwallet/chainfee/rates_test.go b/lnwallet/chainfee/rates_test.go new file mode 100644 index 00000000000..11aa35adf6d --- /dev/null +++ b/lnwallet/chainfee/rates_test.go @@ -0,0 +1,22 @@ +package chainfee + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +// TestSatPerVByteConversion checks that the conversion from sat/vb to either +// sat/kw or sat/kvb is correct. +func TestSatPerVByteConversion(t *testing.T) { + t.Parallel() + + // Create a test fee rate of 1 sat/vb. + rate := SatPerVByte(1) + + // 1 sat/vb should be equal to 1000 sat/kvb. + require.Equal(t, SatPerKVByte(1000), rate.FeePerKVByte()) + + // 1 sat/vb should be equal to 250 sat/kw. + require.Equal(t, SatPerKWeight(250), rate.FeePerKWeight()) +} From e825ed2af18f6638e13f82a636d425e1555865ea Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Wed, 26 Jul 2023 11:04:04 +0200 Subject: [PATCH 3/8] sweep+itest: change `MaxFeeRate` to use `SatPerVbyte` --- itest/lnd_onchain_test.go | 6 ++---- sweep/defaults.go | 7 +++++++ sweep/sweeper.go | 9 ++------- sweep/sweeper_test.go | 2 +- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/itest/lnd_onchain_test.go b/itest/lnd_onchain_test.go index 7ea7d106e09..e7a9b2a913e 100644 --- a/itest/lnd_onchain_test.go +++ b/itest/lnd_onchain_test.go @@ -230,10 +230,8 @@ func runCPFP(ht *lntest.HarnessTest, alice, bob *node.HarnessNode) { // We'll attempt to bump the fee of this transaction by performing a // CPFP from Alice's point of view. bumpFeeReq := &walletrpc.BumpFeeRequest{ - Outpoint: op, - SatPerVbyte: uint64( - sweep.DefaultMaxFeeRate.FeePerKVByte() / 2000, - ), + Outpoint: op, + SatPerVbyte: uint64(sweep.DefaultMaxFeeRate), } bob.RPC.BumpFee(bumpFeeReq) diff --git a/sweep/defaults.go b/sweep/defaults.go index 2f53cdb0a90..3ea49219001 100644 --- a/sweep/defaults.go +++ b/sweep/defaults.go @@ -2,6 +2,8 @@ package sweep import ( "time" + + "github.com/lightningnetwork/lnd/lnwallet/chainfee" ) var ( @@ -9,4 +11,9 @@ var ( // window. The sweep is held back during the batch window to allow more // inputs to be added and thereby lower the fee per input. DefaultBatchWindowDuration = 30 * time.Second + + // DefaultMaxFeeRate is the default maximum fee rate allowed within the + // UtxoSweeper. The current value is equivalent to a fee rate of 1,000 + // sat/vbyte. + DefaultMaxFeeRate chainfee.SatPerVByte = 1e3 ) diff --git a/sweep/sweeper.go b/sweep/sweeper.go index 4ec4154366b..403b4912471 100644 --- a/sweep/sweeper.go +++ b/sweep/sweeper.go @@ -21,11 +21,6 @@ import ( ) const ( - // DefaultMaxFeeRate is the default maximum fee rate allowed within the - // UtxoSweeper. The current value is equivalent to a fee rate of 1,000 - // sat/vbyte. - DefaultMaxFeeRate = chainfee.AbsoluteFeePerKwFloor * 1e3 - // DefaultFeeRateBucketSize is the default size of fee rate buckets // we'll use when clustering inputs into buckets with similar fee rates // within the UtxoSweeper. @@ -288,7 +283,7 @@ type UtxoSweeperConfig struct { // MaxFeeRate is the the maximum fee rate allowed within the // UtxoSweeper. - MaxFeeRate chainfee.SatPerKWeight + MaxFeeRate chainfee.SatPerVByte // FeeRateBucketSize is the default size of fee rate buckets we'll use // when clustering inputs into buckets with similar fee rates within the @@ -484,7 +479,7 @@ func (s *UtxoSweeper) feeRateForPreference( return 0, fmt.Errorf("fee preference resulted in invalid fee "+ "rate %v, minimum is %v", feeRate, s.relayFeeRate) } - if feeRate > s.cfg.MaxFeeRate { + if feeRate > s.cfg.MaxFeeRate.FeePerKWeight() { return 0, fmt.Errorf("fee preference resulted in invalid fee "+ "rate %v, maximum is %v", feeRate, s.cfg.MaxFeeRate) } diff --git a/sweep/sweeper_test.go b/sweep/sweeper_test.go index 13f891857f3..b0ddbee2ed8 100644 --- a/sweep/sweeper_test.go +++ b/sweep/sweeper_test.go @@ -1186,7 +1186,7 @@ func TestBumpFeeRBF(t *testing.T) { // We'll then attempt to bump its fee rate. highFeePref := FeePreference{ConfTarget: 6} - highFeeRate := DefaultMaxFeeRate + highFeeRate := DefaultMaxFeeRate.FeePerKWeight() ctx.estimator.blocksToFee[highFeePref.ConfTarget] = highFeeRate // We should expect to see an error if a fee preference isn't provided. From be0e446e2540f6eea15e147d1c621f4728aea109 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Wed, 12 Jul 2023 21:35:22 +0800 Subject: [PATCH 4/8] multi: make max fee rate configurable --- config.go | 1 + lncfg/sweeper.go | 25 ++++++++++++++++++++++++- sample-lnd.conf | 5 +++++ server.go | 2 +- 4 files changed, 31 insertions(+), 2 deletions(-) diff --git a/config.go b/config.go index 24ea3519368..3e1b4b14e96 100644 --- a/config.go +++ b/config.go @@ -681,6 +681,7 @@ func DefaultConfig() Config { }, Sweeper: &lncfg.Sweeper{ BatchWindowDuration: sweep.DefaultBatchWindowDuration, + MaxFeeRate: sweep.DefaultMaxFeeRate, }, Htlcswitch: &lncfg.Htlcswitch{ MailboxDeliveryTimeout: htlcswitch.DefaultMailboxDeliveryTimeout, diff --git a/lncfg/sweeper.go b/lncfg/sweeper.go index feca51ccb7a..08b12f6dabf 100644 --- a/lncfg/sweeper.go +++ b/lncfg/sweeper.go @@ -3,11 +3,24 @@ package lncfg import ( "fmt" "time" + + "github.com/lightningnetwork/lnd/lnwallet/chainfee" +) + +const ( + // MaxFeeRateFloor is the smallest config value allowed for the max fee + // rate in sat/vb. + MaxFeeRateFloor chainfee.SatPerVByte = 100 + + // MaxAllowedFeeRate is the largest fee rate in sat/vb that we allow + // when configuring the MaxFeeRate. + MaxAllowedFeeRate = 10_000 ) //nolint:lll type Sweeper struct { - BatchWindowDuration time.Duration `long:"batchwindowduration" description:"Duration of the sweep batch window. The sweep is held back during the batch window to allow more inputs to be added and thereby lower the fee per input."` + BatchWindowDuration time.Duration `long:"batchwindowduration" description:"Duration of the sweep batch window. The sweep is held back during the batch window to allow more inputs to be added and thereby lower the fee per input."` + MaxFeeRate chainfee.SatPerVByte `long:"maxfeerate" description:"Maximum fee rate in sat/vb that the sweeper is allowed to use when sweeping funds. Setting this value too low can result in transactions not being confirmed in time, causing HTLCs to expire hence potentially losing funds."` } // Validate checks the values configured for the sweeper. @@ -16,5 +29,15 @@ func (s *Sweeper) Validate() error { return fmt.Errorf("batchwindowduration must be positive") } + // We require the max fee rate to be at least 100 sat/vbyte. + if s.MaxFeeRate < MaxFeeRateFloor { + return fmt.Errorf("maxfeerate must be >= 100 sat/vb") + } + + // We require the max fee rate to be no greater than 10_000 sat/vbyte. + if s.MaxFeeRate > MaxAllowedFeeRate { + return fmt.Errorf("maxfeerate must be <= 10000 sat/vb") + } + return nil } diff --git a/sample-lnd.conf b/sample-lnd.conf index bf6bb47dc46..a491a5d038e 100644 --- a/sample-lnd.conf +++ b/sample-lnd.conf @@ -1578,6 +1578,11 @@ ; window to allow more inputs to be added and thereby lower the fee per input. ; sweeper.batchwindowduration=30s +; The max fee rate in sat/vb which can be used when sweeping funds. Setting +; this value too low can result in transactions not being confirmed in time, +; causing HTLCs to expire hence potentially losing funds. +; sweeper.maxfeerate=1000 + [htlcswitch] diff --git a/server.go b/server.go index f82f9784e6f..8abb196ea77 100644 --- a/server.go +++ b/server.go @@ -1071,7 +1071,7 @@ func newServer(cfg *Config, listenAddrs []net.Addr, MaxInputsPerTx: sweep.DefaultMaxInputsPerTx, MaxSweepAttempts: sweep.DefaultMaxSweepAttempts, NextAttemptDeltaFunc: sweep.DefaultNextAttemptDeltaFunc, - MaxFeeRate: sweep.DefaultMaxFeeRate, + MaxFeeRate: cfg.Sweeper.MaxFeeRate, FeeRateBucketSize: sweep.DefaultFeeRateBucketSize, }) From 19fc6fd4f586b02d1c53f93377744abd323dc062 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Wed, 12 Jul 2023 21:44:01 +0800 Subject: [PATCH 5/8] docs: update release note for maxfeerate --- docs/release-notes/release-notes-0.18.0.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/docs/release-notes/release-notes-0.18.0.md b/docs/release-notes/release-notes-0.18.0.md index 2b566c677ef..85e6df18574 100644 --- a/docs/release-notes/release-notes-0.18.0.md +++ b/docs/release-notes/release-notes-0.18.0.md @@ -21,6 +21,18 @@ # New Features ## Functional Enhancements + +* A new config value, + [sweeper.maxfeerate](https://github.com/lightningnetwork/lnd/pull/7823), is + added so users can specify the max allowed fee rate when sweeping onchain + funds. The default value is 1000 sat/vb. Setting this value below 100 sat/vb + is not allowed, as low fee rate can cause transactions not confirming in + time, which could result in fund loss. + Please note that the actual fee rate to be used is deteremined by the fee + estimator used(for instance `bitcoind`), and this value is a cap on the max + allowed value. So it's expected that this cap is rarely hit unless there's + mempool congestion. + ## RPC Additions ## lncli Additions From f12dce62705dbe2ff00ef3fb44d2f7018eb54c74 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Fri, 18 Aug 2023 18:37:04 +0800 Subject: [PATCH 6/8] sweep: cap to max fee rate instead of failing This commit caps the estimated fee rate at the configged max fee rate instead of returning an error. --- sweep/sweeper.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/sweep/sweeper.go b/sweep/sweeper.go index 403b4912471..33469ad9460 100644 --- a/sweep/sweeper.go +++ b/sweep/sweeper.go @@ -479,9 +479,15 @@ func (s *UtxoSweeper) feeRateForPreference( return 0, fmt.Errorf("fee preference resulted in invalid fee "+ "rate %v, minimum is %v", feeRate, s.relayFeeRate) } + + // If the estimated fee rate is above the maximum allowed fee rate, + // default to the max fee rate. if feeRate > s.cfg.MaxFeeRate.FeePerKWeight() { - return 0, fmt.Errorf("fee preference resulted in invalid fee "+ - "rate %v, maximum is %v", feeRate, s.cfg.MaxFeeRate) + log.Warnf("Estimated fee rate %v exceeds max allowed fee "+ + "rate %v, using max fee rate instead", feeRate, + s.cfg.MaxFeeRate.FeePerKWeight()) + + return s.cfg.MaxFeeRate.FeePerKWeight(), nil } return feeRate, nil From 653be7b7deebc140b4989ba7a73f0aab03ab5768 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Tue, 22 Aug 2023 11:06:51 +0800 Subject: [PATCH 7/8] multi: make sure CPFP won't exceed max allowed fee rate This commit updates the `fee()` method in `weightEstimator` to make sure when doing CPFP we are not exceeding the max allowed fee rate. In order to use the max fee rate, we need to modify several methods to pass the configured value to the estimator. --- itest/lnd_onchain_test.go | 14 ++++++++--- rpcserver.go | 12 ++++----- sweep/sweeper.go | 15 ++++++----- sweep/sweeper_test.go | 2 +- sweep/tx_input_set.go | 10 +++++--- sweep/tx_input_set_test.go | 6 ++--- sweep/txgenerator.go | 17 +++++++------ sweep/txgenerator_test.go | 6 +++-- sweep/walletsweep.go | 15 +++++------ sweep/walletsweep_test.go | 6 ++--- sweep/weight_estimator.go | 31 ++++++++++++++++++++--- sweep/weight_estimator_test.go | 46 +++++++++++++++++++++++++++++++--- 12 files changed, 132 insertions(+), 48 deletions(-) diff --git a/itest/lnd_onchain_test.go b/itest/lnd_onchain_test.go index e7a9b2a913e..4c7a34d4847 100644 --- a/itest/lnd_onchain_test.go +++ b/itest/lnd_onchain_test.go @@ -229,9 +229,12 @@ func runCPFP(ht *lntest.HarnessTest, alice, bob *node.HarnessNode) { // We'll attempt to bump the fee of this transaction by performing a // CPFP from Alice's point of view. + maxFeeRate := uint64(sweep.DefaultMaxFeeRate) bumpFeeReq := &walletrpc.BumpFeeRequest{ - Outpoint: op, - SatPerVbyte: uint64(sweep.DefaultMaxFeeRate), + Outpoint: op, + // We use a higher fee rate than the default max and expect the + // sweeper to cap the fee rate at the max value. + SatPerVbyte: maxFeeRate * 2, } bob.RPC.BumpFee(bumpFeeReq) @@ -249,8 +252,11 @@ func runCPFP(ht *lntest.HarnessTest, alice, bob *node.HarnessNode) { "output txid not matched") require.Equal(ht, pendingSweep.Outpoint.OutputIndex, op.OutputIndex, "output index not matched") - require.Equal(ht, pendingSweep.SatPerVbyte, bumpFeeReq.SatPerVbyte, - "sweep sat per vbyte not matched") + + // Also validate that the fee rate is capped at the max value. + require.Equalf(ht, maxFeeRate, pendingSweep.SatPerVbyte, + "sweep sat per vbyte not matched, want %v, got %v", + maxFeeRate, pendingSweep.SatPerVbyte) // Mine a block to clean up the unconfirmed transactions. ht.MineBlocksAndAssertNumTxes(1, 2) diff --git a/rpcserver.go b/rpcserver.go index 997db710310..4ec1002057b 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -1268,6 +1268,7 @@ func (r *rpcServer) SendCoins(ctx context.Context, var txid *chainhash.Hash wallet := r.server.cc.Wallet + maxFeeRate := r.cfg.Sweeper.MaxFeeRate.FeePerKWeight() // If the send all flag is active, then we'll attempt to sweep all the // coins in the wallet in a single transaction (if possible), @@ -1293,10 +1294,9 @@ func (r *rpcServer) SendCoins(ctx context.Context, // pay to the change address created above if we needed to // reserve any value, the rest will go to targetAddr. sweepTxPkg, err := sweep.CraftSweepAllTx( - feePerKw, uint32(bestHeight), nil, targetAddr, wallet, - wallet, wallet.WalletController, - r.server.cc.FeeEstimator, r.server.cc.Signer, - minConfs, + maxFeeRate, feePerKw, uint32(bestHeight), nil, + targetAddr, wallet, wallet, wallet.WalletController, + r.server.cc.FeeEstimator, r.server.cc.Signer, minConfs, ) if err != nil { return nil, err @@ -1347,8 +1347,8 @@ func (r *rpcServer) SendCoins(ctx context.Context, } sweepTxPkg, err = sweep.CraftSweepAllTx( - feePerKw, uint32(bestHeight), outputs, - targetAddr, wallet, wallet, + maxFeeRate, feePerKw, uint32(bestHeight), + outputs, targetAddr, wallet, wallet, wallet.WalletController, r.server.cc.FeeEstimator, r.server.cc.Signer, minConfs, diff --git a/sweep/sweeper.go b/sweep/sweeper.go index 33469ad9460..72b4f61dcdc 100644 --- a/sweep/sweeper.go +++ b/sweep/sweeper.go @@ -1246,6 +1246,9 @@ func (s *UtxoSweeper) getInputLists(cluster inputCluster, } } + // Convert the max fee rate's unit from sat/vb to sat/kw. + maxFeeRate := s.cfg.MaxFeeRate.FeePerKWeight() + // If there is anything to retry, combine it with the new inputs and // form input sets. var allSets []inputSet @@ -1253,8 +1256,8 @@ func (s *UtxoSweeper) getInputLists(cluster inputCluster, var err error allSets, err = generateInputPartitionings( append(retryInputs, newInputs...), - cluster.sweepFeeRate, s.cfg.MaxInputsPerTx, - s.cfg.Wallet, + cluster.sweepFeeRate, maxFeeRate, + s.cfg.MaxInputsPerTx, s.cfg.Wallet, ) if err != nil { return nil, fmt.Errorf("input partitionings: %v", err) @@ -1263,8 +1266,8 @@ func (s *UtxoSweeper) getInputLists(cluster inputCluster, // Create sets for just the new inputs. newSets, err := generateInputPartitionings( - newInputs, cluster.sweepFeeRate, s.cfg.MaxInputsPerTx, - s.cfg.Wallet, + newInputs, cluster.sweepFeeRate, maxFeeRate, + s.cfg.MaxInputsPerTx, s.cfg.Wallet, ) if err != nil { return nil, fmt.Errorf("input partitionings: %v", err) @@ -1295,7 +1298,7 @@ func (s *UtxoSweeper) sweep(inputs inputSet, feeRate chainfee.SatPerKWeight, // Create sweep tx. tx, err := createSweepTx( inputs, nil, s.currentOutputScript, uint32(currentHeight), - feeRate, s.cfg.Signer, + feeRate, s.cfg.MaxFeeRate.FeePerKWeight(), s.cfg.Signer, ) if err != nil { return fmt.Errorf("create sweep tx: %v", err) @@ -1604,7 +1607,7 @@ func (s *UtxoSweeper) CreateSweepTx(inputs []input.Input, feePref FeePreference, return createSweepTx( inputs, nil, pkScript, currentBlockHeight, feePerKw, - s.cfg.Signer, + s.cfg.MaxFeeRate.FeePerKWeight(), s.cfg.Signer, ) } diff --git a/sweep/sweeper_test.go b/sweep/sweeper_test.go index b0ddbee2ed8..eacf9012ed1 100644 --- a/sweep/sweeper_test.go +++ b/sweep/sweeper_test.go @@ -355,7 +355,7 @@ func assertTxFeeRate(t *testing.T, tx *wire.MsgTx, outputAmt := tx.TxOut[0].Value fee := btcutil.Amount(inputAmt - outputAmt) - _, estimator, err := getWeightEstimate(inputs, nil, 0, changePk) + _, estimator, err := getWeightEstimate(inputs, nil, 0, 0, changePk) require.NoError(t, err) txWeight := estimator.weight() diff --git a/sweep/tx_input_set.go b/sweep/tx_input_set.go index 576471032f3..c42d35f77da 100644 --- a/sweep/tx_input_set.go +++ b/sweep/tx_input_set.go @@ -33,6 +33,9 @@ type txInputSetState struct { // feeRate is the fee rate to use for the sweep transaction. feeRate chainfee.SatPerKWeight + // maxFeeRate is the max allowed fee rate configured by the user. + maxFeeRate chainfee.SatPerKWeight + // inputTotal is the total value of all inputs. inputTotal btcutil.Amount @@ -61,7 +64,7 @@ type txInputSetState struct { // weightEstimate is the (worst case) tx weight with the current set of // inputs. It takes a parameter whether to add a change output or not. func (t *txInputSetState) weightEstimate(change bool) *weightEstimator { - weightEstimate := newWeightEstimator(t.feeRate) + weightEstimate := newWeightEstimator(t.feeRate, t.maxFeeRate) for _, i := range t.inputs { // Can ignore error, because it has already been checked when // calculating the yields. @@ -118,11 +121,12 @@ type txInputSet struct { } // newTxInputSet constructs a new, empty input set. -func newTxInputSet(wallet Wallet, feePerKW chainfee.SatPerKWeight, +func newTxInputSet(wallet Wallet, feePerKW, maxFeeRate chainfee.SatPerKWeight, maxInputs int) *txInputSet { state := txInputSetState{ - feeRate: feePerKW, + feeRate: feePerKW, + maxFeeRate: maxFeeRate, } b := txInputSet{ diff --git a/sweep/tx_input_set_test.go b/sweep/tx_input_set_test.go index c280f9f6a6e..899730d5e3e 100644 --- a/sweep/tx_input_set_test.go +++ b/sweep/tx_input_set_test.go @@ -16,7 +16,7 @@ func TestTxInputSet(t *testing.T) { feeRate = 1000 maxInputs = 10 ) - set := newTxInputSet(nil, feeRate, maxInputs) + set := newTxInputSet(nil, feeRate, 0, maxInputs) // Create a 300 sat input. The fee to sweep this input to a P2WKH output // is 439 sats. That means that this input yields -139 sats and we @@ -65,7 +65,7 @@ func TestTxInputSetFromWallet(t *testing.T) { ) wallet := &mockWallet{} - set := newTxInputSet(wallet, feeRate, maxInputs) + set := newTxInputSet(wallet, feeRate, 0, maxInputs) // Add a 500 sat input to the set. It yields positively, but doesn't // reach the output dust limit. @@ -134,7 +134,7 @@ func TestTxInputSetRequiredOutput(t *testing.T) { feeRate = 1000 maxInputs = 10 ) - set := newTxInputSet(nil, feeRate, maxInputs) + set := newTxInputSet(nil, feeRate, 0, maxInputs) // Attempt to add an input with a required txout below the dust limit. // This should fail since we cannot trim such outputs. diff --git a/sweep/txgenerator.go b/sweep/txgenerator.go index 20e80a00c41..e1f71e7cd98 100644 --- a/sweep/txgenerator.go +++ b/sweep/txgenerator.go @@ -39,7 +39,7 @@ type inputSet []input.Input // inputs are skipped. No input sets with a total value after fees below the // dust limit are returned. func generateInputPartitionings(sweepableInputs []txInput, - feePerKW chainfee.SatPerKWeight, maxInputsPerTx int, + feePerKW, maxFeeRate chainfee.SatPerKWeight, maxInputsPerTx int, wallet Wallet) ([]inputSet, error) { // Sort input by yield. We will start constructing input sets starting @@ -87,7 +87,9 @@ func generateInputPartitionings(sweepableInputs []txInput, // Start building a set of positive-yield tx inputs under the // condition that the tx will be published with the specified // fee rate. - txInputs := newTxInputSet(wallet, feePerKW, maxInputsPerTx) + txInputs := newTxInputSet( + wallet, feePerKW, maxFeeRate, maxInputsPerTx, + ) // From the set of sweepable inputs, keep adding inputs to the // input set until the tx output value no longer goes up or the @@ -137,11 +139,11 @@ func generateInputPartitionings(sweepableInputs []txInput, // sending any leftover change to the change script. func createSweepTx(inputs []input.Input, outputs []*wire.TxOut, changePkScript []byte, currentBlockHeight uint32, - feePerKw chainfee.SatPerKWeight, signer input.Signer) (*wire.MsgTx, - error) { + feePerKw, maxFeeRate chainfee.SatPerKWeight, + signer input.Signer) (*wire.MsgTx, error) { inputs, estimator, err := getWeightEstimate( - inputs, outputs, feePerKw, changePkScript, + inputs, outputs, feePerKw, maxFeeRate, changePkScript, ) if err != nil { return nil, err @@ -319,7 +321,8 @@ func createSweepTx(inputs []input.Input, outputs []*wire.TxOut, // getWeightEstimate returns a weight estimate for the given inputs. // Additionally, it returns counts for the number of csv and cltv inputs. func getWeightEstimate(inputs []input.Input, outputs []*wire.TxOut, - feeRate chainfee.SatPerKWeight, outputPkScript []byte) ([]input.Input, + feeRate, maxFeeRate chainfee.SatPerKWeight, + outputPkScript []byte) ([]input.Input, *weightEstimator, error) { // We initialize a weight estimator so we can accurately asses the @@ -327,7 +330,7 @@ func getWeightEstimate(inputs []input.Input, outputs []*wire.TxOut, // // TODO(roasbeef): can be more intelligent about buffering outputs to // be more efficient on-chain. - weightEstimate := newWeightEstimator(feeRate) + weightEstimate := newWeightEstimator(feeRate, maxFeeRate) // Our sweep transaction will always pay to the given set of outputs. for _, o := range outputs { diff --git a/sweep/txgenerator_test.go b/sweep/txgenerator_test.go index cb575e5a649..48dcacd4994 100644 --- a/sweep/txgenerator_test.go +++ b/sweep/txgenerator_test.go @@ -50,7 +50,9 @@ func TestWeightEstimate(t *testing.T) { 0x00, 0x00, 0x00, 0x00, } - _, estimator, err := getWeightEstimate(inputs, nil, 0, changePkScript) + _, estimator, err := getWeightEstimate( + inputs, nil, 0, 0, changePkScript, + ) require.NoError(t, err) weight := int64(estimator.weight()) @@ -151,7 +153,7 @@ func testUnknownScriptInner(t *testing.T, pkscript []byte, expectFail bool) { )) } - _, _, err := getWeightEstimate(inputs, nil, 0, pkscript) + _, _, err := getWeightEstimate(inputs, nil, 0, 0, pkscript) if expectFail { require.Error(t, err) } else { diff --git a/sweep/walletsweep.go b/sweep/walletsweep.go index f806d32764f..3ba6f0a35bf 100644 --- a/sweep/walletsweep.go +++ b/sweep/walletsweep.go @@ -180,11 +180,12 @@ type DeliveryAddr struct { // output, as specified by the change address. The sweep transaction will be // crafted with the target fee rate, and will use the utxoSource and // outpointLocker as sources for wallet funds. -func CraftSweepAllTx(feeRate chainfee.SatPerKWeight, blockHeight uint32, - deliveryAddrs []DeliveryAddr, changeAddr btcutil.Address, - coinSelectLocker CoinSelectionLocker, utxoSource UtxoSource, - outpointLocker OutpointLocker, feeEstimator chainfee.Estimator, - signer input.Signer, minConfs int32) (*WalletSweepPackage, error) { +func CraftSweepAllTx(feeRate, maxFeeRate chainfee.SatPerKWeight, + blockHeight uint32, deliveryAddrs []DeliveryAddr, + changeAddr btcutil.Address, coinSelectLocker CoinSelectionLocker, + utxoSource UtxoSource, outpointLocker OutpointLocker, + feeEstimator chainfee.Estimator, signer input.Signer, + minConfs int32) (*WalletSweepPackage, error) { // TODO(roasbeef): turn off ATPL as well when available? @@ -320,8 +321,8 @@ func CraftSweepAllTx(feeRate chainfee.SatPerKWeight, blockHeight uint32, // Finally, we'll ask the sweeper to craft a sweep transaction which // respects our fee preference and targets all the UTXOs of the wallet. sweepTx, err := createSweepTx( - inputsToSweep, txOuts, changePkScript, blockHeight, feeRate, - signer, + inputsToSweep, txOuts, changePkScript, blockHeight, + feeRate, maxFeeRate, signer, ) if err != nil { unlockOutputs() diff --git a/sweep/walletsweep_test.go b/sweep/walletsweep_test.go index f2196fc8998..3b0d9f61c9f 100644 --- a/sweep/walletsweep_test.go +++ b/sweep/walletsweep_test.go @@ -297,7 +297,7 @@ func TestCraftSweepAllTxCoinSelectFail(t *testing.T) { utxoLocker := newMockOutpointLocker() _, err := CraftSweepAllTx( - 0, 10, nil, nil, coinSelectLocker, utxoSource, utxoLocker, nil, + 0, 0, 10, nil, nil, coinSelectLocker, utxoSource, utxoLocker, nil, nil, 0, ) @@ -323,7 +323,7 @@ func TestCraftSweepAllTxUnknownWitnessType(t *testing.T) { utxoLocker := newMockOutpointLocker() _, err := CraftSweepAllTx( - 0, 10, nil, nil, coinSelectLocker, utxoSource, utxoLocker, nil, + 0, 0, 10, nil, nil, coinSelectLocker, utxoSource, utxoLocker, nil, nil, 0, ) @@ -358,7 +358,7 @@ func TestCraftSweepAllTx(t *testing.T) { utxoLocker := newMockOutpointLocker() sweepPkg, err := CraftSweepAllTx( - 0, 10, nil, deliveryAddr, coinSelectLocker, utxoSource, + 0, 0, 10, nil, deliveryAddr, coinSelectLocker, utxoSource, utxoLocker, feeEstimator, signer, 0, ) require.NoError(t, err, "unable to make sweep tx") diff --git a/sweep/weight_estimator.go b/sweep/weight_estimator.go index c09adabdc8c..7dae25d80fc 100644 --- a/sweep/weight_estimator.go +++ b/sweep/weight_estimator.go @@ -16,13 +16,19 @@ type weightEstimator struct { parents map[chainhash.Hash]struct{} parentsFee btcutil.Amount parentsWeight int64 + + // maxFeeRate is the max allowed fee rate configured by the user. + maxFeeRate chainfee.SatPerKWeight } // newWeightEstimator instantiates a new sweeper weight estimator. -func newWeightEstimator(feeRate chainfee.SatPerKWeight) *weightEstimator { +func newWeightEstimator( + feeRate, maxFeeRate chainfee.SatPerKWeight) *weightEstimator { + return &weightEstimator{ - feeRate: feeRate, - parents: make(map[chainhash.Hash]struct{}), + feeRate: feeRate, + maxFeeRate: maxFeeRate, + parents: make(map[chainhash.Hash]struct{}), } } @@ -119,5 +125,24 @@ func (w *weightEstimator) fee() btcutil.Amount { fee = childFee } + // Exit early if maxFeeRate is not set. + if w.maxFeeRate == 0 { + return fee + } + + // Clamp the fee to the max fee rate. + maxFee := w.maxFeeRate.FeeForWeight(childWeight) + if fee > maxFee { + // Calculate the effective fee rate for logging. + childFeeRate := chainfee.SatPerKWeight( + fee * 1000 / btcutil.Amount(childWeight), + ) + log.Warnf("Child fee rate %v exceeds max allowed fee rate %v, "+ + "returning fee %v instead of %v", childFeeRate, + w.maxFeeRate, maxFee, fee) + + fee = maxFee + } + return fee } diff --git a/sweep/weight_estimator_test.go b/sweep/weight_estimator_test.go index 1952f531e86..350da9af3e9 100644 --- a/sweep/weight_estimator_test.go +++ b/sweep/weight_estimator_test.go @@ -17,7 +17,7 @@ import ( func TestWeightEstimator(t *testing.T) { testFeeRate := chainfee.SatPerKWeight(20000) - w := newWeightEstimator(testFeeRate) + w := newWeightEstimator(testFeeRate, 0) // Add an input without unconfirmed parent tx. input1 := input.MakeBaseInput( @@ -81,6 +81,46 @@ func TestWeightEstimator(t *testing.T) { require.Equal(t, expectedFee, w.fee()) } +// TestWeightEstimatorMaxFee tests that the weight estimator correctly caps the +// fee at the maximum allowed fee. +func TestWeightEstimatorMaxFee(t *testing.T) { + t.Parallel() + + testFeeRate := chainfee.SatPerKWeight(9_000) + maxFeeRate := chainfee.SatPerKWeight(10_000) + + w := newWeightEstimator(testFeeRate, maxFeeRate) + + // Define a parent transaction that pays a fee of 1000 sat/kw. + parentTxLowFee := &input.TxInfo{ + Weight: 100, + Fee: 100, + } + + // Add an output of the low-fee parent tx above. + childInput := input.MakeBaseInput( + &wire.OutPoint{}, input.CommitmentAnchor, + &input.SignDescriptor{}, 0, parentTxLowFee, + ) + require.NoError(t, w.add(&childInput)) + + // The child weight should be 322 weight uints. + const childWeight = 322 + require.Equal(t, childWeight, w.weight()) + + // Check the fee is capped at the maximum allowed fee. The + // calculations, + // + // totalWeight = childWeight + parentWeight = 422 + // fee = totalWeight * testFeeRate - parentsFee = + // 422 * 9_000 / 1000 - 100 = 3598 + // maxFee = childWeight * maxFeeRate = 422 * 10_000 / 1000 = 3220 + // + // Thus we cap at the maxFee. + expectedFee := maxFeeRate.FeeForWeight(childWeight) + require.Equal(t, expectedFee, w.fee()) +} + // TestWeightEstimatorAddOutput tests that adding the raw P2WKH output to the // estimator yield the same result as an estimated add. func TestWeightEstimatorAddOutput(t *testing.T) { @@ -100,10 +140,10 @@ func TestWeightEstimatorAddOutput(t *testing.T) { Value: 10000, } - w1 := newWeightEstimator(testFeeRate) + w1 := newWeightEstimator(testFeeRate, 0) w1.addOutput(txOut) - w2 := newWeightEstimator(testFeeRate) + w2 := newWeightEstimator(testFeeRate, 0) w2.addP2WKHOutput() // Estimate hhould be the same. From 4622ea92c675290ddaef3092c24616a2953742b9 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Tue, 22 Aug 2023 12:42:02 +0800 Subject: [PATCH 8/8] rpcserver+sweep: fix linter re unused params --- rpcserver.go | 5 ++--- sweep/walletsweep.go | 3 +-- sweep/walletsweep_test.go | 7 +++---- 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/rpcserver.go b/rpcserver.go index 4ec1002057b..01d5c5fe410 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -1296,7 +1296,7 @@ func (r *rpcServer) SendCoins(ctx context.Context, sweepTxPkg, err := sweep.CraftSweepAllTx( maxFeeRate, feePerKw, uint32(bestHeight), nil, targetAddr, wallet, wallet, wallet.WalletController, - r.server.cc.FeeEstimator, r.server.cc.Signer, minConfs, + r.server.cc.Signer, minConfs, ) if err != nil { return nil, err @@ -1350,8 +1350,7 @@ func (r *rpcServer) SendCoins(ctx context.Context, maxFeeRate, feePerKw, uint32(bestHeight), outputs, targetAddr, wallet, wallet, wallet.WalletController, - r.server.cc.FeeEstimator, r.server.cc.Signer, - minConfs, + r.server.cc.Signer, minConfs, ) if err != nil { return nil, err diff --git a/sweep/walletsweep.go b/sweep/walletsweep.go index 3ba6f0a35bf..831b0151e48 100644 --- a/sweep/walletsweep.go +++ b/sweep/walletsweep.go @@ -184,8 +184,7 @@ func CraftSweepAllTx(feeRate, maxFeeRate chainfee.SatPerKWeight, blockHeight uint32, deliveryAddrs []DeliveryAddr, changeAddr btcutil.Address, coinSelectLocker CoinSelectionLocker, utxoSource UtxoSource, outpointLocker OutpointLocker, - feeEstimator chainfee.Estimator, signer input.Signer, - minConfs int32) (*WalletSweepPackage, error) { + signer input.Signer, minConfs int32) (*WalletSweepPackage, error) { // TODO(roasbeef): turn off ATPL as well when available? diff --git a/sweep/walletsweep_test.go b/sweep/walletsweep_test.go index 3b0d9f61c9f..4a7ceec7a74 100644 --- a/sweep/walletsweep_test.go +++ b/sweep/walletsweep_test.go @@ -297,7 +297,7 @@ func TestCraftSweepAllTxCoinSelectFail(t *testing.T) { utxoLocker := newMockOutpointLocker() _, err := CraftSweepAllTx( - 0, 0, 10, nil, nil, coinSelectLocker, utxoSource, utxoLocker, nil, + 0, 0, 10, nil, nil, coinSelectLocker, utxoSource, utxoLocker, nil, 0, ) @@ -323,7 +323,7 @@ func TestCraftSweepAllTxUnknownWitnessType(t *testing.T) { utxoLocker := newMockOutpointLocker() _, err := CraftSweepAllTx( - 0, 0, 10, nil, nil, coinSelectLocker, utxoSource, utxoLocker, nil, + 0, 0, 10, nil, nil, coinSelectLocker, utxoSource, utxoLocker, nil, 0, ) @@ -348,7 +348,6 @@ func TestCraftSweepAllTx(t *testing.T) { // First, we'll make a mock signer along with a fee estimator, We'll // use zero fees to we can assert a precise output value. signer := &mock.DummySigner{} - feeEstimator := newMockFeeEstimator(0, 0) // For our UTXO source, we'll pass in all the UTXOs that we know of, // other than the final one which is of an unknown witness type. @@ -359,7 +358,7 @@ func TestCraftSweepAllTx(t *testing.T) { sweepPkg, err := CraftSweepAllTx( 0, 0, 10, nil, deliveryAddr, coinSelectLocker, utxoSource, - utxoLocker, feeEstimator, signer, 0, + utxoLocker, signer, 0, ) require.NoError(t, err, "unable to make sweep tx")