From 8804947179118b223fe79da0a4ce6d20d19cb0ed Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Thu, 11 Apr 2024 17:08:50 +0800 Subject: [PATCH] walletrpc+sweep: refactor `BumpFee` to properly handle sweep request --- lnrpc/walletrpc/walletkit_server.go | 256 +++++++++++++++++++++------- sweep/fee_bumper.go | 8 +- sweep/sweeper.go | 19 +-- 3 files changed, 204 insertions(+), 79 deletions(-) diff --git a/lnrpc/walletrpc/walletkit_server.go b/lnrpc/walletrpc/walletkit_server.go index 8c94b29f0..11bd13451 100644 --- a/lnrpc/walletrpc/walletkit_server.go +++ b/lnrpc/walletrpc/walletkit_server.go @@ -30,6 +30,8 @@ import ( base "github.com/btcsuite/btcwallet/wallet" "github.com/btcsuite/btcwallet/wtxmgr" "github.com/grpc-ecosystem/grpc-gateway/v2/runtime" + "github.com/lightningnetwork/lnd/contractcourt" + "github.com/lightningnetwork/lnd/fn" "github.com/lightningnetwork/lnd/input" "github.com/lightningnetwork/lnd/keychain" "github.com/lightningnetwork/lnd/labels" @@ -951,6 +953,131 @@ func UnmarshallOutPoint(op *lnrpc.OutPoint) (*wire.OutPoint, error) { }, nil } +// validateBumpFeeRequest makes sure the deprecated fields are not used when +// the new fields are set. +func validateBumpFeeRequest(in *BumpFeeRequest) ( + fn.Option[chainfee.SatPerKWeight], bool, error) { + + // Get the specified fee rate if set. + satPerKwOpt := fn.None[chainfee.SatPerKWeight]() + + // We only allow using either the deprecated field or the new field. + switch { + case in.SatPerByte != 0 && in.SatPerVbyte != 0: + return satPerKwOpt, false, fmt.Errorf("either SatPerByte or " + + "SatPerVbyte should be set, but not both") + + case in.SatPerByte != 0: + satPerKw := chainfee.SatPerVByte( + in.SatPerByte, + ).FeePerKWeight() + satPerKwOpt = fn.Some(satPerKw) + + case in.SatPerVbyte != 0: + satPerKw := chainfee.SatPerVByte( + in.SatPerVbyte, + ).FeePerKWeight() + satPerKwOpt = fn.Some(satPerKw) + } + + var immediate bool + switch { + case in.Force && in.Immediate: + return satPerKwOpt, false, fmt.Errorf("either Force or " + + "Immediate should be set, but not both") + + case in.Force: + immediate = in.Force + + case in.Immediate: + immediate = in.Immediate + } + + return satPerKwOpt, immediate, nil +} + +// prepareSweepParams creates the sweep params to be used for the sweeper. It +// returns the new params and a bool indicating whether this is an existing +// input. +func (w *WalletKit) prepareSweepParams(in *BumpFeeRequest, + op wire.OutPoint, currentHeight int32) (sweep.Params, bool, error) { + + // Return an error if both deprecated and new fields are used. + feerate, immediate, err := validateBumpFeeRequest(in) + if err != nil { + return sweep.Params{}, false, err + } + + // Get the current pending inputs. + inputMap, err := w.cfg.Sweeper.PendingInputs() + if err != nil { + return sweep.Params{}, false, fmt.Errorf("unable to get "+ + "pending inputs: %w", err) + } + + // Find the pending input. + // + // TODO(yy): act differently based on the state of the input? + inp, ok := inputMap[op] + + if !ok { + // NOTE: if this input doesn't exist and the new budget is not + // specified, the params would have a zero budget. + params := sweep.Params{ + Immediate: immediate, + StartingFeeRate: feerate, + Budget: btcutil.Amount(in.Budget), + } + if in.TargetConf != 0 { + params.DeadlineHeight = fn.Some( + int32(in.TargetConf) + currentHeight, + ) + } + + return params, ok, nil + } + + // Find the existing budget used for this input. Note that this value + // must be greater than zero. + budget := inp.Params.Budget + + // Set the new budget if specified. + if in.Budget != 0 { + budget = btcutil.Amount(in.Budget) + } + + // For an existing input, we assign it first, then overwrite it if + // a deadline is requested. + deadline := inp.Params.DeadlineHeight + + // Set the deadline if target conf is specified. + // + // TODO(yy): upgrade `falafel` so we can make this field optional. Atm + // we cannot distinguish between user's not setting the field and + // setting it to 0. + if in.TargetConf != 0 { + deadline = fn.Some(int32(in.TargetConf) + currentHeight) + } + + // Prepare the new sweep params. + // + // NOTE: if this input doesn't exist and the new budget is not + // specified, the params would have a zero budget. + params := sweep.Params{ + Immediate: immediate, + StartingFeeRate: feerate, + DeadlineHeight: deadline, + Budget: budget, + } + + if ok { + log.Infof("[BumpFee]: bumping fee for existing input=%v, old "+ + "params=%v, new params=%v", op, inp.Params, params) + } + + return params, ok, nil +} + // BumpFee allows bumping the fee rate of an arbitrary input. A fee preference // can be expressed either as a specific fee rate or a delta of blocks in which // the output should be swept on-chain within. If a fee preference is not @@ -965,67 +1092,82 @@ func (w *WalletKit) BumpFee(ctx context.Context, return nil, err } - // We only allow using either the deprecated field or the new field. - if in.SatPerByte != 0 && in.SatPerVbyte != 0 { - return nil, fmt.Errorf("either SatPerByte or " + - "SatPerVbyte should be set, but not both") + // Get the current height so we can calculate the deadline height. + _, currentHeight, err := w.cfg.Chain.GetBestBlock() + if err != nil { + return nil, fmt.Errorf("unable to retrieve current height: %w", + err) } - // Construct the request's fee preference. - satPerKw := chainfee.SatPerKVByte(in.SatPerVbyte * 1000).FeePerKWeight() - if in.SatPerByte != 0 { - satPerKw = chainfee.SatPerKVByte( - in.SatPerByte * 1000, - ).FeePerKWeight() - } - feePreference := sweep.FeeEstimateInfo{ - ConfTarget: uint32(in.TargetConf), - FeeRate: satPerKw, - } - - // We'll attempt to bump the fee of the input through the UtxoSweeper. - // If it is currently attempting to sweep the input, then it'll simply - // bump its fee, which will result in a replacement transaction (RBF) - // being broadcast. If it is not aware of the input however, - // lnwallet.ErrNotMine is returned. - params := sweep.Params{ - Fee: feePreference, - Immediate: in.Immediate, - } - - _, err = w.cfg.Sweeper.UpdateParams(*op, params) - switch err { - case nil: - return &BumpFeeResponse{ - Status: "Successfully registered rbf-tx with sweeper", - }, nil - case lnwallet.ErrNotMine: - break - default: + // We now create a new sweeping params and update it in the sweeper. + // This will complicate the RBF conditions if this input has already + // been offered to sweeper before and it has already been included in a + // tx with other inputs. If this is the case, two results are possible: + // - either this input successfully RBFed the existing tx, or, + // - the budget of this input was not enough to RBF the existing tx. + params, existing, err := w.prepareSweepParams(in, *op, currentHeight) + if err != nil { return nil, err } - log.Debugf("Attempting to CPFP outpoint %s", op) + // If this input exists, we will update its params. + if existing { + _, err = w.cfg.Sweeper.UpdateParams(*op, params) + if err != nil { + return nil, err + } - // Since we're unable to perform a bump through RBF, we'll assume the - // user is attempting to bump an unconfirmed transaction's fee rate by + return &BumpFeeResponse{ + Status: "Successfully registered rbf-tx with sweeper", + }, nil + } + + // Otherwise, create a new sweeping request for this input. + err = w.sweepNewInput(op, uint32(currentHeight), params) + if err != nil { + return nil, err + } + + return &BumpFeeResponse{ + Status: "Successfully registered CPFP-tx with the sweeper", + }, nil +} + +// sweepNewInput handles the case where an input is seen the first time by the +// sweeper. It will fetch the output from the wallet and construct an input and +// offer it to the sweeper. +// +// NOTE: if the budget is not set, the default budget ratio is used. +func (w *WalletKit) sweepNewInput(op *wire.OutPoint, currentHeight uint32, + params sweep.Params) error { + + log.Debugf("Attempting to sweep outpoint %s", op) + + // Since the sweeper is not aware of the input, we'll assume the user + // is attempting to bump an unconfirmed transaction's fee rate by // sweeping an output within it under control of the wallet with a - // higher fee rate, essentially performing a Child-Pays-For-Parent - // (CPFP). + // higher fee rate. In this case, this will be a CPFP. // // We'll gather all of the information required by the UtxoSweeper in // order to sweep the output. utxo, err := w.cfg.Wallet.FetchInputInfo(op) if err != nil { - return nil, err + return err } // We're only able to bump the fee of unconfirmed transactions. if utxo.Confirmations > 0 { - return nil, errors.New("unable to bump fee of a confirmed " + + return errors.New("unable to bump fee of a confirmed " + "transaction") } + // If there's no budget set, use the default value. + if params.Budget == 0 { + params.Budget = utxo.Value.MulF64( + contractcourt.DefaultBudgetRatio, + ) + } + signDesc := &input.SignDescriptor{ Output: &wire.TxOut{ PkScript: utxo.PkScript, @@ -1044,32 +1186,18 @@ func (w *WalletKit) BumpFee(ctx context.Context, witnessType = input.TaprootPubKeySpend signDesc.HashType = txscript.SigHashDefault default: - return nil, fmt.Errorf("unknown input witness %v", op) + return fmt.Errorf("unknown input witness %v", op) } - // We'll use the current height as the height hint since we're dealing - // with an unconfirmed transaction. - _, currentHeight, err := w.cfg.Chain.GetBestBlock() - if err != nil { - return nil, fmt.Errorf("unable to retrieve current height: %w", - err) + log.Infof("[BumpFee]: bumping fee for new input=%v, params=%v", op, + params) + + inp := input.NewBaseInput(op, witnessType, signDesc, currentHeight) + if _, err = w.cfg.Sweeper.SweepInput(inp, params); err != nil { + return err } - inp := input.NewBaseInput( - op, witnessType, signDesc, uint32(currentHeight), - ) - - sweepParams := sweep.Params{ - Fee: feePreference, - Immediate: in.Immediate, - } - if _, err = w.cfg.Sweeper.SweepInput(inp, sweepParams); err != nil { - return nil, err - } - - return &BumpFeeResponse{ - Status: "Successfully registered cpfp-tx with the sweeper", - }, nil + return nil } // ListSweeps returns a list of the sweeps that our node has published. diff --git a/sweep/fee_bumper.go b/sweep/fee_bumper.go index 0c9ea7e04..c052bb47b 100644 --- a/sweep/fee_bumper.go +++ b/sweep/fee_bumper.go @@ -490,12 +490,12 @@ func (t *TxPublisher) createAndCheckTx(req *BumpRequest, f FeeFunction) ( req.Inputs, req.DeliveryAddress, f.FeeRate(), ) if err != nil { - return nil, 0, fmt.Errorf("create sweep tx: %w", err) + return nil, fee, fmt.Errorf("create sweep tx: %w", err) } // Sanity check the budget still covers the fee. if fee > req.Budget { - return nil, 0, fmt.Errorf("%w: budget=%v, fee=%v", + return nil, fee, fmt.Errorf("%w: budget=%v, fee=%v", ErrNotEnoughBudget, req.Budget, fee) } @@ -522,8 +522,8 @@ func (t *TxPublisher) createAndCheckTx(req *BumpRequest, f FeeFunction) ( return tx, fee, nil } - return nil, 0, fmt.Errorf("tx=%v failed mempool check: %w", tx.TxHash(), - err) + return nil, fee, fmt.Errorf("tx=%v failed mempool check: %w", + tx.TxHash(), err) } // broadcast takes a monitored tx and publishes it to the network. Prior to the diff --git a/sweep/sweeper.go b/sweep/sweeper.go index cfedfb010..3db1b5eba 100644 --- a/sweep/sweeper.go +++ b/sweep/sweeper.go @@ -1087,14 +1087,6 @@ func (s *UtxoSweeper) handlePendingSweepsReq( func (s *UtxoSweeper) UpdateParams(input wire.OutPoint, params Params) (chan Result, error) { - // Ensure the client provided a sane fee preference. - _, err := params.Fee.Estimate( - s.cfg.FeeEstimator, s.cfg.MaxFeeRate.FeePerKWeight(), - ) - if err != nil { - return nil, err - } - responseChan := make(chan *updateResp, 1) select { case s.updateReqs <- &updateReq{ @@ -1140,9 +1132,14 @@ func (s *UtxoSweeper) handleUpdateReq(req *updateReq) ( // Create the updated parameters struct. Leave the exclusive group // unchanged. - newParams := sweeperInput.params - newParams.Fee = req.params.Fee - newParams.Immediate = req.params.Immediate + newParams := Params{ + Fee: req.params.Fee, + StartingFeeRate: req.params.StartingFeeRate, + Immediate: req.params.Immediate, + Budget: req.params.Budget, + DeadlineHeight: req.params.DeadlineHeight, + ExclusiveGroup: sweeperInput.params.ExclusiveGroup, + } log.Debugf("Updating parameters for %v(state=%v) from (%v) to (%v)", req.input, sweeperInput.state, sweeperInput.params, newParams)