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

@@ -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),
+		// 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)

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,
+			maxFeeRate, feePerKw, uint32(bestHeight), nil,
-			wallet, wallet.WalletController,
+			targetAddr, wallet, wallet, wallet.WalletController,
-			r.server.cc.FeeEstimator, r.server.cc.Signer,
+			r.server.cc.FeeEstimator, r.server.cc.Signer, minConfs,
-			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,
+				maxFeeRate, feePerKw, uint32(bestHeight),
-				targetAddr, wallet, wallet,
+				outputs, targetAddr, wallet, wallet,
 				wallet.WalletController,
 				r.server.cc.FeeEstimator, r.server.cc.Signer,
 				minConfs,

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,
+			cluster.sweepFeeRate, maxFeeRate,
-			s.cfg.Wallet,
+			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,
+		newInputs, cluster.sweepFeeRate, maxFeeRate,
-		s.cfg.Wallet,
+		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,
 	)
 }
 

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()

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,
+		maxFeeRate: maxFeeRate,
 	}
 
 	b := txInputSet{

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.

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,
+	feePerKw, maxFeeRate chainfee.SatPerKWeight,
-	error) {
+	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 {

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 {

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,
+func CraftSweepAllTx(feeRate, maxFeeRate chainfee.SatPerKWeight,
-	deliveryAddrs []DeliveryAddr, changeAddr btcutil.Address,
+	blockHeight uint32, deliveryAddrs []DeliveryAddr,
-	coinSelectLocker CoinSelectionLocker, utxoSource UtxoSource,
+	changeAddr btcutil.Address, coinSelectLocker CoinSelectionLocker,
-	outpointLocker OutpointLocker, feeEstimator chainfee.Estimator,
+	utxoSource UtxoSource, outpointLocker OutpointLocker,
-	signer input.Signer, minConfs int32) (*WalletSweepPackage, error) {
+	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,
+		inputsToSweep, txOuts, changePkScript, blockHeight,
-		signer,
+		feeRate, maxFeeRate, signer,
 	)
 	if err != nil {
 		unlockOutputs()

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")

sweep/weight_estimator.go

@@ -16,12 +16,18 @@ 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,
+		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
 }

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.