From 24fa35ec8015073cb792190d755f9dae10f4a649 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Tue, 22 Aug 2023 11:06:51 +0800 Subject: [PATCH] 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 e7a9b2a91..4c7a34d48 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 26062d77c..dadaa51c6 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 44e1656b4..c11895198 100644 --- a/sweep/sweeper.go +++ b/sweep/sweeper.go @@ -1245,6 +1245,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 @@ -1252,8 +1255,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) @@ -1262,8 +1265,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) @@ -1294,7 +1297,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) @@ -1603,7 +1606,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 b0ddbee2e..eacf9012e 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 576471032..c42d35f77 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 c280f9f6a..899730d5e 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 20e80a00c..e1f71e7cd 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 cb575e5a6..48dcacd49 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 f806d3276..3ba6f0a35 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 f2196fc89..3b0d9f61c 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 c09adabdc..7dae25d80 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 1952f531e..350da9af3 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.