From 3c4fd1b484735f40b4c0d040c5b8f2f6409b8259 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Wed, 19 Mar 2025 01:41:35 +0800 Subject: [PATCH 01/13] sweep: refactor `AddWalletInputs` by adding `addWalletInput` A minor refactor to prepare for upcoming changes. --- sweep/tx_input_set.go | 40 ++++++++++++++++++++++++++----------- sweep/tx_input_set_test.go | 41 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 12 deletions(-) diff --git a/sweep/tx_input_set.go b/sweep/tx_input_set.go index b80d52b0e..ff3b7f309 100644 --- a/sweep/tx_input_set.go +++ b/sweep/tx_input_set.go @@ -244,6 +244,28 @@ func (b *BudgetInputSet) addInput(input SweeperInput) { b.inputs = append(b.inputs, &input) } +// addWalletInput takes a wallet UTXO and adds it as an input to be used as +// budget for the input set. +func (b *BudgetInputSet) addWalletInput(utxo *lnwallet.Utxo) error { + input, err := createWalletTxInput(utxo) + if err != nil { + return err + } + + pi := SweeperInput{ + Input: input, + params: Params{ + DeadlineHeight: fn.Some(b.deadlineHeight), + }, + } + b.addInput(pi) + + log.Debugf("Added wallet input to input set: op=%v, amt=%v", + pi.OutPoint(), utxo.Value) + + return nil +} + // NeedWalletInput returns true if the input set needs more wallet inputs. // // A set may need wallet inputs when it has a required output or its total @@ -326,6 +348,11 @@ func (b *BudgetInputSet) AddWalletInputs(wallet Wallet) error { return fmt.Errorf("list unspent witness: %w", err) } + // Exit early if there are no wallet UTXOs. + if len(utxos) == 0 { + return ErrNotEnoughInputs + } + // Sort the UTXOs by putting smaller values at the start of the slice // to avoid locking large UTXO for sweeping. // @@ -342,22 +369,11 @@ func (b *BudgetInputSet) AddWalletInputs(wallet Wallet) error { // Add wallet inputs to the set until the specified budget is covered. for _, utxo := range utxos { - input, err := createWalletTxInput(utxo) + err := b.addWalletInput(utxo) if err != nil { return err } - pi := SweeperInput{ - Input: input, - params: Params{ - DeadlineHeight: fn.Some(b.deadlineHeight), - }, - } - b.addInput(pi) - - log.Debugf("Added wallet input to input set: op=%v, amt=%v", - pi.OutPoint(), utxo.Value) - // Return if we've reached the minimum output amount. if !b.NeedWalletInput() { return nil diff --git a/sweep/tx_input_set_test.go b/sweep/tx_input_set_test.go index 73f056a96..0ea056e49 100644 --- a/sweep/tx_input_set_test.go +++ b/sweep/tx_input_set_test.go @@ -133,6 +133,47 @@ func TestBudgetInputSetAddInput(t *testing.T) { require.Equal(t, btcutil.Amount(200), set.Budget()) } +// TestAddWalletInput asserts `addWalletInput` successfully converts a wallet +// UTXO into a `SweeperInput` with the correct deadline. +func TestAddWalletInput(t *testing.T) { + t.Parallel() + + // Create a testing deadline. + deadline := int32(1000) + + // Initialize an empty input set. + set := &BudgetInputSet{ + deadlineHeight: deadline, + } + + // Create an utxo with unknown address type to trigger an error. + utxo := &lnwallet.Utxo{ + AddressType: lnwallet.UnknownAddressType, + } + + // Check that the error is returned from addWalletInput. + err := set.addWalletInput(utxo) + require.Error(t, err) + + // Create a wallet utxo. + utxo = &lnwallet.Utxo{ + AddressType: lnwallet.WitnessPubKey, + Value: 1000, + } + + // Check that no error is returned from addWalletInput. + err = set.addWalletInput(utxo) + require.NoError(t, err) + + // Check the input has been added to the set. + require.Len(t, set.inputs, 1) + + // Assert the wallet input is added using the set's deadline. + inp := set.inputs[0] + require.True(t, inp.params.DeadlineHeight.IsSome()) + require.Equal(t, deadline, inp.params.DeadlineHeight.UnsafeFromSome()) +} + // TestNeedWalletInput checks that NeedWalletInput correctly determines if a // wallet input is needed. func TestNeedWalletInput(t *testing.T) { From 861dc145bf0767481e74a1d12b83503635a0d43e Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Wed, 19 Mar 2025 17:32:42 +0800 Subject: [PATCH 02/13] sweep: create sweep tx even the budget cannot be met We now always create the sweeping tx even though the budget cannot be covered so we don't miss the deadline. Note that the fee bump will fail once the provided wallet input cannot cover the increase fees, which is fine as these inputs will be marked as failed and be retried again in the next block. When that happens, if there are new wallet UTXOs, a new batch will be created to perform the fee bump. --- sweep/fee_bumper.go | 16 +++++++++++++--- sweep/tx_input_set.go | 18 +++++++----------- sweep/tx_input_set_test.go | 26 ++++++++++++-------------- 3 files changed, 32 insertions(+), 28 deletions(-) diff --git a/sweep/fee_bumper.go b/sweep/fee_bumper.go index b081fe479..99360d6fb 100644 --- a/sweep/fee_bumper.go +++ b/sweep/fee_bumper.go @@ -1114,6 +1114,14 @@ func (t *TxPublisher) handleInitialTxError(r *monitorRecord, err error) { case errors.Is(err, ErrZeroFeeRateDelta): result.Event = TxFailed + // When the error is due to not enough inputs to cover the budget, we'll + // send a TxFailed event so these inputs can be retried when the wallet + // has more UTXOs. + case errors.Is(err, ErrNotEnoughInputs), + errors.Is(err, ErrNotEnoughBudget): + + result.Event = TxFailed + // When there are missing inputs, we'll create a TxUnknownSpend bump // result here so the rest of the inputs can be retried. case errors.Is(err, ErrInputMissing): @@ -1757,9 +1765,11 @@ func prepareSweepTx(inputs []input.Input, changePkScript lnwallet.AddrWithKey, // Make sure total output amount is less than total input amount. if requiredOutput+txFee > totalInput { - return 0, noChange, noLocktime, fmt.Errorf("insufficient "+ - "input to create sweep tx: input_sum=%v, "+ - "output_sum=%v", totalInput, requiredOutput+txFee) + log.Errorf("Insufficient input to create sweep tx: "+ + "input_sum=%v, output_sum=%v", totalInput, + requiredOutput+txFee) + + return 0, noChange, noLocktime, ErrNotEnoughInputs } // The value remaining after the required output and fees is the diff --git a/sweep/tx_input_set.go b/sweep/tx_input_set.go index ff3b7f309..db4b893ed 100644 --- a/sweep/tx_input_set.go +++ b/sweep/tx_input_set.go @@ -296,7 +296,9 @@ func (b *BudgetInputSet) NeedWalletInput() bool { } // Get the amount left after covering the input's own budget. - // This amount can then be lent to the above input. + // This amount can then be lent to the above input. For a wallet + // input, its `Budget` is set to zero, which means the whole + // input can be borrowed to cover the budget. budget := inp.params.Budget output := btcutil.Amount(inp.SignDesc().Output.Value) budgetBorrowable += output - budget @@ -350,7 +352,7 @@ func (b *BudgetInputSet) AddWalletInputs(wallet Wallet) error { // Exit early if there are no wallet UTXOs. if len(utxos) == 0 { - return ErrNotEnoughInputs + return fmt.Errorf("%w: empty wallet", ErrNotEnoughInputs) } // Sort the UTXOs by putting smaller values at the start of the slice @@ -362,11 +364,6 @@ func (b *BudgetInputSet) AddWalletInputs(wallet Wallet) error { return utxos[i].Value < utxos[j].Value }) - // Make a copy of the current inputs. If the wallet doesn't have enough - // utxos to cover the budget, we will revert the current set to its - // original state by removing the added wallet inputs. - originalInputs := b.copyInputs() - // Add wallet inputs to the set until the specified budget is covered. for _, utxo := range utxos { err := b.addWalletInput(utxo) @@ -380,11 +377,10 @@ func (b *BudgetInputSet) AddWalletInputs(wallet Wallet) error { } } - // The wallet doesn't have enough utxos to cover the budget. Revert the - // input set to its original state. - b.inputs = originalInputs + log.Warn("Not enough wallet UTXOs to cover the budget, sweeping " + + "anyway...") - return ErrNotEnoughInputs + return nil } // Budget returns the total budget of the set. diff --git a/sweep/tx_input_set_test.go b/sweep/tx_input_set_test.go index 0ea056e49..6e91a3fb1 100644 --- a/sweep/tx_input_set_test.go +++ b/sweep/tx_input_set_test.go @@ -386,12 +386,12 @@ func TestNeedWalletInput(t *testing.T) { } } -// TestAddWalletInputReturnErr tests the three possible errors returned from +// TestAddWalletInputsReturnErr tests the three possible errors returned from // AddWalletInputs: // - error from ListUnspentWitnessFromDefaultAccount. // - error from createWalletTxInput. // - error when wallet doesn't have utxos. -func TestAddWalletInputReturnErr(t *testing.T) { +func TestAddWalletInputsReturnErr(t *testing.T) { t.Parallel() wallet := &MockWallet{} @@ -436,10 +436,9 @@ func TestAddWalletInputReturnErr(t *testing.T) { require.ErrorIs(t, err, ErrNotEnoughInputs) } -// TestAddWalletInputNotEnoughInputs checks that when there are not enough -// wallet utxos, an error is returned and the budget set is reset to its -// initial state. -func TestAddWalletInputNotEnoughInputs(t *testing.T) { +// TestAddWalletInputsNotEnoughInputs checks that when there are not enough +// wallet utxos, no error is returned as long as the wallet is not empty. +func TestAddWalletInputsNotEnoughInputs(t *testing.T) { t.Parallel() wallet := &MockWallet{} @@ -476,19 +475,18 @@ func TestAddWalletInputNotEnoughInputs(t *testing.T) { // Initialize an input set with the pending input. set := BudgetInputSet{inputs: []*SweeperInput{pi}} - // Add wallet inputs to the input set, which should give us an error as - // the wallet cannot cover the budget. + // Add wallet inputs to the input set, which should return no error + // although the wallet cannot cover the budget. err := set.AddWalletInputs(wallet) - require.ErrorIs(t, err, ErrNotEnoughInputs) + require.NoError(t, err) - // Check that the budget set is reverted to its initial state. - require.Len(t, set.inputs, 1) - require.Equal(t, pi, set.inputs[0]) + // Check that the budget set is updated. + require.Len(t, set.inputs, 2) } -// TestAddWalletInputSuccess checks that when there are enough wallet utxos, +// TestAddWalletInputsSuccess checks that when there are enough wallet utxos, // they are added to the input set. -func TestAddWalletInputSuccess(t *testing.T) { +func TestAddWalletInputsSuccess(t *testing.T) { t.Parallel() wallet := &MockWallet{} From 6dbf4ce4706dca2a27b79838e639c40552fa13f1 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Wed, 19 Mar 2025 18:02:35 +0800 Subject: [PATCH 03/13] sweep: add method `calculateRetryFeeRate` A minor refactor to prepare the upcoming changes. --- sweep/fee_bumper.go | 109 +++++++++++++++++++++++++------------------- 1 file changed, 63 insertions(+), 46 deletions(-) diff --git a/sweep/fee_bumper.go b/sweep/fee_bumper.go index 99360d6fb..d69158eab 100644 --- a/sweep/fee_bumper.go +++ b/sweep/fee_bumper.go @@ -1267,56 +1267,17 @@ func (t *TxPublisher) createUnknownSpentBumpResult( SpentInputs: r.spentInputs, } - // Get the fee function, which will be used to decided the next fee rate - // to use if the sweeper decides to retry sweeping this input. - feeFunc := r.feeFunction - - // When the record is failed before the initial broadcast is attempted, - // it will have a nil fee func. In this case, we'll create the fee func - // here. - // - // NOTE: Since the current record is failed and will be deleted, we - // don't need to update the record on this fee function. We only need - // the fee rate data so the sweeper can pick up where we left off. - if feeFunc == nil { - f, err := t.initializeFeeFunction(r.req) - // TODO(yy): The only error we would receive here is when the - // pkScript is not recognized by the weightEstimator. What we - // should do instead is to check the pkScript immediately after - // receiving a sweep request so we don't need to check it again, - // which will also save us from error checking from several - // callsites. - if err != nil { - log.Errorf("Failed to create fee func for record %v: "+ - "%v", r.requestID, err) - - // Overwrite the event and error so the sweeper will - // remove this input. - result.Event = TxFatal - result.Err = err - - return result - } - - feeFunc = f - } - - // Since the sweeping tx has been replaced by another party's tx, we - // missed this block window to increase its fee rate. To make sure the - // fee rate stays in the initial line, we now ask the fee function to - // give us the next fee rate as if the sweeping tx were RBFed. This new - // fee rate will be used as the starting fee rate if the upper system - // decides to continue sweeping the rest of the inputs. - _, err := feeFunc.Increment() + // Calculate the next fee rate for the retry. + feeRate, err := t.calculateRetryFeeRate(r) if err != nil { - // The fee function has reached its max position - nothing we - // can do here other than letting the user increase the budget. - log.Errorf("Failed to calculate the next fee rate for "+ - "Record(%v): %v", r.requestID, err) + // Overwrite the event and error so the sweeper will + // remove this input. + result.Event = TxFatal + result.Err = err } // Attach the new fee rate to be used for the next sweeping attempt. - result.FeeRate = feeFunc.FeeRate() + result.FeeRate = feeRate return result } @@ -1893,3 +1854,59 @@ func (t *TxPublisher) handleReplacementTxError(r *monitorRecord, return result } + +// calculateRetryFeeRate calculates a new fee rate to be used as the starting +// fee rate for the next sweep attempt if the inputs are to be retried. When the +// fee function is nil it will be created here, and an error is returned if the +// fee func cannot be initialized. +func (t *TxPublisher) calculateRetryFeeRate( + r *monitorRecord) (chainfee.SatPerKWeight, error) { + + // Get the fee function, which will be used to decided the next fee rate + // to use if the sweeper decides to retry sweeping this input. + feeFunc := r.feeFunction + + // When the record is failed before the initial broadcast is attempted, + // it will have a nil fee func. In this case, we'll create the fee func + // here. + // + // NOTE: Since the current record is failed and will be deleted, we + // don't need to update the record on this fee function. We only need + // the fee rate data so the sweeper can pick up where we left off. + if feeFunc == nil { + f, err := t.initializeFeeFunction(r.req) + + // TODO(yy): The only error we would receive here is when the + // pkScript is not recognized by the weightEstimator. What we + // should do instead is to check the pkScript immediately after + // receiving a sweep request so we don't need to check it again, + // which will also save us from error checking from several + // callsites. + if err != nil { + log.Errorf("Failed to create fee func for record %v: "+ + "%v", r.requestID, err) + + return 0, err + } + + feeFunc = f + } + + // Since we failed to sweep the inputs, either the sweeping tx has been + // replaced by another party's tx, or the current output values cannot + // cover the budget, we missed this block window to increase its fee + // rate. To make sure the fee rate stays in the initial line, we now ask + // the fee function to give us the next fee rate as if the sweeping tx + // were RBFed. This new fee rate will be used as the starting fee rate + // if the upper system decides to continue sweeping the rest of the + // inputs. + _, err := feeFunc.Increment() + if err != nil { + // The fee function has reached its max position - nothing we + // can do here other than letting the user increase the budget. + log.Errorf("Failed to calculate the next fee rate for "+ + "Record(%v): %v", r.requestID, err) + } + + return feeFunc.FeeRate(), nil +} From eea3561eeadd1fefcdcfcaa8a15f47e40af06e89 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Thu, 20 Mar 2025 21:57:08 +0800 Subject: [PATCH 04/13] sweep+itest: return next retry fee rate in `TxFailed` event We now return the next retry fee rate in `TxFailed` event in `TxPublisher`. When handling the event, `UtxoSweeper` will update the inputs to make sure the starting fee rate is set before attempting the next sweep. --- sweep/fee_bumper.go | 54 +++++++++++++++++++++++++++++----------- sweep/fee_bumper_test.go | 1 + sweep/sweeper.go | 27 ++++++++++++-------- sweep/sweeper_test.go | 38 ++++++++++++++++++---------- 4 files changed, 82 insertions(+), 38 deletions(-) diff --git a/sweep/fee_bumper.go b/sweep/fee_bumper.go index d69158eab..1602165ea 100644 --- a/sweep/fee_bumper.go +++ b/sweep/fee_bumper.go @@ -1103,25 +1103,36 @@ func (t *TxPublisher) handleInitialTxError(r *monitorRecord, err error) { case errors.Is(err, ErrTxNoOutput): result.Event = TxFailed - // When the error is due to budget being used up, we'll send a TxFailed - // so these inputs can be retried with a different group in the next - // block. - case errors.Is(err, ErrMaxPosition): - result.Event = TxFailed - // When the error is due to zero fee rate delta, we'll send a TxFailed // so these inputs can be retried in the next block. case errors.Is(err, ErrZeroFeeRateDelta): result.Event = TxFailed - // When the error is due to not enough inputs to cover the budget, we'll - // send a TxFailed event so these inputs can be retried when the wallet - // has more UTXOs. + // When the error is due to budget being used up, we'll send a TxFailed + // so these inputs can be retried with a different group in the next + // block. + case errors.Is(err, ErrMaxPosition): + fallthrough + + // If the tx doesn't not have enough budget, or if the inputs amounts + // are not sufficient to cover the budget, we will return a TxFailed + // event so the sweeper can handle it by re-clustering the utxos. case errors.Is(err, ErrNotEnoughInputs), errors.Is(err, ErrNotEnoughBudget): result.Event = TxFailed + // Calculate the starting fee rate to be used when retry + // sweeping these inputs. + feeRate, err := t.calculateRetryFeeRate(r) + if err != nil { + result.Event = TxFatal + result.Err = err + } + + // Attach the new fee rate. + result.FeeRate = feeRate + // When there are missing inputs, we'll create a TxUnknownSpend bump // result here so the rest of the inputs can be retried. case errors.Is(err, ErrInputMissing): @@ -1832,18 +1843,33 @@ func (t *TxPublisher) handleReplacementTxError(r *monitorRecord, return fn.Some(*bumpResult) } - // If the error is not fee related, we will return a `TxFailed` event - // so this input can be retried. + // Return a failed event to retry the sweep. + event := TxFailed + + // Calculate the next fee rate for the retry. + feeRate, ferr := t.calculateRetryFeeRate(r) + if ferr != nil { + // If there's an error with the fee calculation, we need to + // abort the sweep. + event = TxFatal + } + + // If the error is not fee related, we will return a `TxFailed` event so + // this input can be retried. result := fn.Some(BumpResult{ - Event: TxFailed, + Event: event, Tx: oldTx, Err: err, requestID: r.requestID, + FeeRate: feeRate, }) - // If the tx doesn't not have enought budget, we will return a result so + // If the tx doesn't not have enough budget, or if the inputs amounts + // are not sufficient to cover the budget, we will return a result so // the sweeper can handle it by re-clustering the utxos. - if errors.Is(err, ErrNotEnoughBudget) { + if errors.Is(err, ErrNotEnoughBudget) || + errors.Is(err, ErrNotEnoughInputs) { + log.Warnf("Fail to fee bump tx %v: %v", oldTx.TxHash(), err) return result } diff --git a/sweep/fee_bumper_test.go b/sweep/fee_bumper_test.go index 64695dbfe..aa5561b5a 100644 --- a/sweep/fee_bumper_test.go +++ b/sweep/fee_bumper_test.go @@ -1134,6 +1134,7 @@ func TestCreateAnPublishFail(t *testing.T) { // Create a test feerate and return it from the mock fee function. feerate := chainfee.SatPerKWeight(1000) m.feeFunc.On("FeeRate").Return(feerate) + m.feeFunc.On("Increment").Return(true, nil).Once() // Create a testing monitor record. req := createTestBumpRequest() diff --git a/sweep/sweeper.go b/sweep/sweeper.go index 69cf9c5d8..d240f5b34 100644 --- a/sweep/sweeper.go +++ b/sweep/sweeper.go @@ -951,7 +951,9 @@ func (s *UtxoSweeper) markInputsPublished(tr *TxRecord, set InputSet) error { } // markInputsPublishFailed marks the list of inputs as failed to be published. -func (s *UtxoSweeper) markInputsPublishFailed(set InputSet) { +func (s *UtxoSweeper) markInputsPublishFailed(set InputSet, + feeRate chainfee.SatPerKWeight) { + // Reschedule sweep. for _, inp := range set.Inputs() { op := inp.OutPoint() @@ -978,6 +980,15 @@ func (s *UtxoSweeper) markInputsPublishFailed(set InputSet) { // Update the input's state. pi.state = PublishFailed + + log.Debugf("Input(%v): updating params: starting fee rate "+ + "[%v -> %v]", op, pi.params.StartingFeeRate, + feeRate) + + // Update the input using the fee rate specified from the + // BumpResult, which should be the starting fee rate to use for + // the next sweeping attempt. + pi.params.StartingFeeRate = fn.Some(feeRate) } } @@ -1699,7 +1710,7 @@ func (s *UtxoSweeper) handleBumpEventTxFailed(resp *bumpResp) { // the inputs specified by the set. // // TODO(yy): should we also remove the failed tx from db? - s.markInputsPublishFailed(resp.set) + s.markInputsPublishFailed(resp.set, resp.result.FeeRate) } // handleBumpEventTxReplaced handles the case where the sweeping tx has been @@ -1948,7 +1959,7 @@ func (s *UtxoSweeper) handleUnknownSpendTx(inp *SweeperInput, tx *wire.MsgTx) { func (s *UtxoSweeper) handleBumpEventTxUnknownSpend(r *bumpResp) { // Mark the inputs as publish failed, which means they will be retried // later. - s.markInputsPublishFailed(r.set) + s.markInputsPublishFailed(r.set, r.result.FeeRate) // Get all the inputs that are not spent in the current sweeping tx. spentInputs := r.result.SpentInputs @@ -1982,15 +1993,9 @@ func (s *UtxoSweeper) handleBumpEventTxUnknownSpend(r *bumpResp) { continue } - log.Debugf("Input(%v): updating params: starting fee rate "+ - "[%v -> %v], immediate [%v -> true]", op, - input.params.StartingFeeRate, r.result.FeeRate, - input.params.Immediate) + log.Debugf("Input(%v): updating params: immediate [%v -> true]", + op, r.result.FeeRate, input.params.Immediate) - // Update the input using the fee rate specified from the - // BumpResult, which should be the starting fee rate to use for - // the next sweeping attempt. - input.params.StartingFeeRate = fn.Some(r.result.FeeRate) input.params.Immediate = true inputsToRetry = append(inputsToRetry, input) } diff --git a/sweep/sweeper_test.go b/sweep/sweeper_test.go index d1fbee1f6..d97fd9925 100644 --- a/sweep/sweeper_test.go +++ b/sweep/sweeper_test.go @@ -232,39 +232,51 @@ func TestMarkInputsPublishFailed(t *testing.T) { inputPublishFailed, inputSwept, inputExcluded, inputFatal, }) + feeRate := chainfee.SatPerKWeight(1000) + // Mark the test inputs. We expect the non-exist input and the // inputInit to be skipped, and the final input to be marked as // published. - s.markInputsPublishFailed(set) + s.markInputsPublishFailed(set, feeRate) // We expect unchanged number of pending inputs. require.Len(s.inputs, 7) // We expect the init input's state to stay unchanged. - require.Equal(Init, - s.inputs[inputInit.OutPoint()].state) + pi := s.inputs[inputInit.OutPoint()] + require.Equal(Init, pi.state) + require.True(pi.params.StartingFeeRate.IsNone()) // We expect the pending-publish input's is now marked as publish // failed. - require.Equal(PublishFailed, - s.inputs[inputPendingPublish.OutPoint()].state) + pi = s.inputs[inputPendingPublish.OutPoint()] + require.Equal(PublishFailed, pi.state) + require.Equal(feeRate, pi.params.StartingFeeRate.UnsafeFromSome()) // We expect the published input's is now marked as publish failed. - require.Equal(PublishFailed, - s.inputs[inputPublished.OutPoint()].state) + pi = s.inputs[inputPublished.OutPoint()] + require.Equal(PublishFailed, pi.state) + require.Equal(feeRate, pi.params.StartingFeeRate.UnsafeFromSome()) // We expect the publish failed input to stay unchanged. - require.Equal(PublishFailed, - s.inputs[inputPublishFailed.OutPoint()].state) + pi = s.inputs[inputPublishFailed.OutPoint()] + require.Equal(PublishFailed, pi.state) + require.True(pi.params.StartingFeeRate.IsNone()) // We expect the swept input to stay unchanged. - require.Equal(Swept, s.inputs[inputSwept.OutPoint()].state) + pi = s.inputs[inputSwept.OutPoint()] + require.Equal(Swept, pi.state) + require.True(pi.params.StartingFeeRate.IsNone()) // We expect the excluded input to stay unchanged. - require.Equal(Excluded, s.inputs[inputExcluded.OutPoint()].state) + pi = s.inputs[inputExcluded.OutPoint()] + require.Equal(Excluded, pi.state) + require.True(pi.params.StartingFeeRate.IsNone()) - // We expect the failed input to stay unchanged. - require.Equal(Fatal, s.inputs[inputFatal.OutPoint()].state) + // We expect the fatal input to stay unchanged. + pi = s.inputs[inputFatal.OutPoint()] + require.Equal(Fatal, pi.state) + require.True(pi.params.StartingFeeRate.IsNone()) // Assert mocked statements are executed as expected. mockStore.AssertExpectations(t) From 4abc1461ad05aed29cae5e4849b08829f40650c5 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Thu, 20 Mar 2025 21:57:19 +0800 Subject: [PATCH 05/13] itest: refactor `runBumpFee` to fix a flake Make sure we assertPendingSweepResp in a wait call to wait for the updated resp. --- itest/lnd_sweep_test.go | 117 +++++++++++++++++++++++++++------------- 1 file changed, 81 insertions(+), 36 deletions(-) diff --git a/itest/lnd_sweep_test.go b/itest/lnd_sweep_test.go index 63c489853..8533b74d0 100644 --- a/itest/lnd_sweep_test.go +++ b/itest/lnd_sweep_test.go @@ -1606,40 +1606,62 @@ func runBumpFee(ht *lntest.HarnessTest, alice *node.HarnessNode) { assertPendingSweepResp := func(broadcastAttempts uint32, budget uint64, deadline uint32, startingFeeRate uint64) *wire.MsgTx { - // Alice should still have one pending sweep. - pendingSweep := ht.AssertNumPendingSweeps(alice, 1)[0] + err := wait.NoError(func() error { + // Alice should still have one pending sweep. + ps := ht.AssertNumPendingSweeps(alice, 1)[0] - // Validate all fields returned from `PendingSweeps` are as - // expected. - require.Equal(ht, op.TxidBytes, pendingSweep.Outpoint.TxidBytes) - require.Equal(ht, op.OutputIndex, - pendingSweep.Outpoint.OutputIndex) - require.Equal(ht, walletrpc.WitnessType_TAPROOT_PUB_KEY_SPEND, - pendingSweep.WitnessType) - require.EqualValuesf(ht, value, pendingSweep.AmountSat, - "amount not matched: want=%d, got=%d", value, - pendingSweep.AmountSat) - require.True(ht, pendingSweep.Immediate) + // Validate all fields returned from `PendingSweeps` are + // as expected. + // + // These fields should stay the same during the test so + // we assert the values without wait. + require.Equal(ht, op.TxidBytes, ps.Outpoint.TxidBytes) + require.Equal(ht, op.OutputIndex, + ps.Outpoint.OutputIndex) + require.Equal(ht, + walletrpc.WitnessType_TAPROOT_PUB_KEY_SPEND, + ps.WitnessType) + require.EqualValuesf(ht, value, ps.AmountSat, + "amount not matched: want=%d, got=%d", value, + ps.AmountSat) - require.Equal(ht, broadcastAttempts, - pendingSweep.BroadcastAttempts) - require.EqualValuesf(ht, budget, pendingSweep.Budget, - "budget not matched: want=%d, got=%d", budget, - pendingSweep.Budget) + // The following fields can change during the test so we + // return an error if they don't match, which will be + // checked again in this wait call. + if ps.Immediate != true { + return fmt.Errorf("Immediate should be true") + } - // Since the request doesn't specify a deadline, we expect the - // existing deadline to be used. - require.Equalf(ht, deadline, pendingSweep.DeadlineHeight, - "deadline height not matched: want=%d, got=%d", - deadline, pendingSweep.DeadlineHeight) + if broadcastAttempts != ps.BroadcastAttempts { + return fmt.Errorf("broadcastAttempts not "+ + "matched: want=%d, got=%d", + broadcastAttempts, ps.BroadcastAttempts) + } + if budget != ps.Budget { + return fmt.Errorf("budget not matched: "+ + "want=%d, got=%d", budget, ps.Budget) + } - // Since the request specifies a starting fee rate, we expect - // that to be used as the starting fee rate. - require.Equalf(ht, startingFeeRate, - pendingSweep.RequestedSatPerVbyte, "requested "+ - "starting fee rate not matched: want=%d, "+ - "got=%d", startingFeeRate, - pendingSweep.RequestedSatPerVbyte) + // Since the request doesn't specify a deadline, we + // expect the existing deadline to be used. + if deadline != ps.DeadlineHeight { + return fmt.Errorf("deadline height not "+ + "matched: want=%d, got=%d", deadline, + ps.DeadlineHeight) + } + + // Since the request specifies a starting fee rate, we + // expect that to be used as the starting fee rate. + if startingFeeRate != ps.RequestedSatPerVbyte { + return fmt.Errorf("requested starting fee "+ + "rate not matched: want=%d, got=%d", + startingFeeRate, + ps.RequestedSatPerVbyte) + } + + return nil + }, wait.DefaultTimeout) + require.NoError(ht, err, "timeout checking pending sweep") // We expect to see Alice's original tx and her CPFP tx in the // mempool. @@ -1879,7 +1901,7 @@ func runBumpFee(ht *lntest.HarnessTest, alice *node.HarnessNode) { // Finally, we test the behavior of lowering the fee rate. The fee func // that has, // - starting fee rate: 1 sat/vbyte. - // - deadline: 1008. + // - deadline: 1. // - budget: 1000 sats. bumpFeeReq = &walletrpc.BumpFeeRequest{ Outpoint: op, @@ -1888,22 +1910,45 @@ func runBumpFee(ht *lntest.HarnessTest, alice *node.HarnessNode) { SatPerVbyte: startFeeRate, // The budget and the deadline delta must be set together. Budget: smallBudget, - DeadlineDelta: uint32(sweep.DefaultDeadlineDelta), + DeadlineDelta: 1, } alice.RPC.BumpFee(bumpFeeReq) + // Calculate the ending fee rate, which is used in the above fee bump + // when fee function's max posistion is reached. + txWeight := ht.CalculateTxWeight(sweepTx6) + endingFeeRate := chainfee.NewSatPerKWeight( + btcutil.Amount(smallBudget), txWeight, + ) + + // Since the fee function has been maxed out, the starting fee rate for + // the next sweep attempt should be the ending fee rate. + // + // TODO(yy): The weight estimator used in the sweeper gives a different + // result than the weight calculated here, which is the result from + // `blockchain.GetTransactionWeight`. For this particular tx: + // - result from the `weightEstimator`: 445 wu + // - result from `GetTransactionWeight`: 444 wu + // + // This means the fee rates are different, + // - `weightEstimator`: 2247 sat/kw, or 8 sat/vb (8.988 round down) + // - here we have 2252 sat/kw, or 9 sat/vb (9.008 round down) + // + // We should investigate and check whether if it's possible to make the + // `weightEstimator` more accurate. + expectedStartFeeRate := uint64(endingFeeRate.FeePerVByte()) - 1 + // Assert the pending sweep is created with the expected values: // - broadcast attempts: 7. - // - starting fee rate: 1 sat/vbyte. - // - deadline: 1008. + // - starting fee rate: 8 sat/vbyte. + // - deadline: 1. // - budget: 1000 sats. sweepTx7 := assertPendingSweepResp( - 7, smallBudget, deadline, startFeeRate, + 7, smallBudget, uint32(currentHeight+1), expectedStartFeeRate, ) // Since this budget is too small to cover the RBF, we expect the // sweeping attempt to fail. - // require.Equal(ht, sweepTx6.TxHash(), sweepTx7.TxHash(), "tx6 should "+ "not be replaced: tx6=%v, tx7=%v", sweepTx6.TxHash(), sweepTx7.TxHash()) From 883381d266a780c6e60d9163639099357f81e46e Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Wed, 19 Mar 2025 17:23:30 +0800 Subject: [PATCH 06/13] lntest+itest: return the tx from `FundCoins` This is used is a following test. --- itest/lnd_funding_test.go | 16 ++++++++++--- lntest/harness.go | 50 ++++++++++++++++++--------------------- 2 files changed, 36 insertions(+), 30 deletions(-) diff --git a/itest/lnd_funding_test.go b/itest/lnd_funding_test.go index 4a984a06e..52f0f7805 100644 --- a/itest/lnd_funding_test.go +++ b/itest/lnd_funding_test.go @@ -8,6 +8,7 @@ import ( "github.com/btcsuite/btcd/btcutil" "github.com/btcsuite/btcd/chaincfg/chainhash" "github.com/btcsuite/btcd/txscript" + "github.com/btcsuite/btcd/wire" "github.com/lightningnetwork/lnd/chainreg" "github.com/lightningnetwork/lnd/funding" "github.com/lightningnetwork/lnd/labels" @@ -466,20 +467,29 @@ func runChannelFundingInputTypes(ht *lntest.HarnessTest, alice, burnAddr = "bcrt1qxsnqpdc842lu8c0xlllgvejt6rhy49u6fmpgyz" ) - fundMixed := func(amt btcutil.Amount, target *node.HarnessNode) { + fundMixed := func(amt btcutil.Amount, + target *node.HarnessNode) *wire.MsgTx { + ht.FundCoins(amt/5, target) ht.FundCoins(amt/5, target) ht.FundCoinsP2TR(amt/5, target) ht.FundCoinsP2TR(amt/5, target) ht.FundCoinsP2TR(amt/5, target) + + return nil } - fundMultipleP2TR := func(amt btcutil.Amount, target *node.HarnessNode) { + fundMultipleP2TR := func(amt btcutil.Amount, + target *node.HarnessNode) *wire.MsgTx { + ht.FundCoinsP2TR(amt/4, target) ht.FundCoinsP2TR(amt/4, target) ht.FundCoinsP2TR(amt/4, target) ht.FundCoinsP2TR(amt/4, target) + + return nil } - fundWithTypes := []func(amt btcutil.Amount, target *node.HarnessNode){ + fundWithTypes := []func(amt btcutil.Amount, + target *node.HarnessNode) *wire.MsgTx{ ht.FundCoins, ht.FundCoinsNP2WKH, ht.FundCoinsP2TR, fundMixed, fundMultipleP2TR, } diff --git a/lntest/harness.go b/lntest/harness.go index c43cdfd45..d0a36f6dd 100644 --- a/lntest/harness.go +++ b/lntest/harness.go @@ -2,7 +2,6 @@ package lntest import ( "context" - "encoding/hex" "fmt" "strings" "testing" @@ -1425,7 +1424,7 @@ func (h *HarnessTest) IsNeutrinoBackend() bool { // transaction that pays to the target should confirm. For neutrino backend, // the `confirmed` param is ignored. func (h *HarnessTest) fundCoins(amt btcutil.Amount, target *node.HarnessNode, - addrType lnrpc.AddressType, confirmed bool) { + addrType lnrpc.AddressType, confirmed bool) *wire.MsgTx { initialBalance := target.RPC.WalletBalance() @@ -1443,25 +1442,14 @@ func (h *HarnessTest) fundCoins(amt btcutil.Amount, target *node.HarnessNode, PkScript: addrScript, Value: int64(amt), } - h.miner.SendOutput(output, defaultMinerFeeRate) + txid := h.miner.SendOutput(output, defaultMinerFeeRate) - // Encode the pkScript in hex as this the format that it will be - // returned via rpc. - expPkScriptStr := hex.EncodeToString(addrScript) + // Get the funding tx. + tx := h.GetRawTransaction(*txid) + msgTx := tx.MsgTx() - // Now, wait for ListUnspent to show the unconfirmed transaction - // containing the correct pkscript. - // // Since neutrino doesn't support unconfirmed outputs, skip this check. if !h.IsNeutrinoBackend() { - utxos := h.AssertNumUTXOsUnconfirmed(target, 1) - - // Assert that the lone unconfirmed utxo contains the same - // pkscript as the output generated above. - pkScriptStr := utxos[0].PkScript - require.Equal(h, pkScriptStr, expPkScriptStr, - "pkscript mismatch") - expectedBalance := btcutil.Amount( initialBalance.UnconfirmedBalance, ) + amt @@ -1472,48 +1460,56 @@ func (h *HarnessTest) fundCoins(amt btcutil.Amount, target *node.HarnessNode, // the target node's unconfirmed balance reflects the expected balance // and exit. if !confirmed { - return + return msgTx } // Otherwise, we'll generate 1 new blocks to ensure the output gains a // sufficient number of confirmations and wait for the balance to // reflect what's expected. - h.MineBlocksAndAssertNumTxes(1, 1) + h.MineBlockWithTx(msgTx) expectedBalance := btcutil.Amount(initialBalance.ConfirmedBalance) + amt h.WaitForBalanceConfirmed(target, expectedBalance) + + return msgTx } // FundCoins attempts to send amt satoshis from the internal mining node to the // targeted lightning node using a P2WKH address. 1 blocks are mined after in // order to confirm the transaction. -func (h *HarnessTest) FundCoins(amt btcutil.Amount, hn *node.HarnessNode) { - h.fundCoins(amt, hn, lnrpc.AddressType_WITNESS_PUBKEY_HASH, true) +func (h *HarnessTest) FundCoins(amt btcutil.Amount, + hn *node.HarnessNode) *wire.MsgTx { + + return h.fundCoins(amt, hn, lnrpc.AddressType_WITNESS_PUBKEY_HASH, true) } // FundCoinsUnconfirmed attempts to send amt satoshis from the internal mining // node to the targeted lightning node using a P2WKH address. No blocks are // mined after and the UTXOs are unconfirmed. func (h *HarnessTest) FundCoinsUnconfirmed(amt btcutil.Amount, - hn *node.HarnessNode) { + hn *node.HarnessNode) *wire.MsgTx { - h.fundCoins(amt, hn, lnrpc.AddressType_WITNESS_PUBKEY_HASH, false) + return h.fundCoins( + amt, hn, lnrpc.AddressType_WITNESS_PUBKEY_HASH, false, + ) } // FundCoinsNP2WKH attempts to send amt satoshis from the internal mining node // to the targeted lightning node using a NP2WKH address. func (h *HarnessTest) FundCoinsNP2WKH(amt btcutil.Amount, - target *node.HarnessNode) { + target *node.HarnessNode) *wire.MsgTx { - h.fundCoins(amt, target, lnrpc.AddressType_NESTED_PUBKEY_HASH, true) + return h.fundCoins( + amt, target, lnrpc.AddressType_NESTED_PUBKEY_HASH, true, + ) } // FundCoinsP2TR attempts to send amt satoshis from the internal mining node to // the targeted lightning node using a P2TR address. func (h *HarnessTest) FundCoinsP2TR(amt btcutil.Amount, - target *node.HarnessNode) { + target *node.HarnessNode) *wire.MsgTx { - h.fundCoins(amt, target, lnrpc.AddressType_TAPROOT_PUBKEY, true) + return h.fundCoins(amt, target, lnrpc.AddressType_TAPROOT_PUBKEY, true) } // FundNumCoins attempts to send the given number of UTXOs from the internal From 64f7a7f3d039d2dd0bac596db203b6af00ed1ab1 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Wed, 19 Mar 2025 17:24:20 +0800 Subject: [PATCH 07/13] lntest+itest: update block height in `MineBlockWithTx` Make sure we update the harness's current height and assert nodes have been synced. Also fixes some typo found. --- itest/lnd_multi-hop_force_close_test.go | 2 +- itest/lnd_sweep_test.go | 4 ++-- lntest/harness_miner.go | 10 +++++++++- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/itest/lnd_multi-hop_force_close_test.go b/itest/lnd_multi-hop_force_close_test.go index 84f8c8d44..c542c81ac 100644 --- a/itest/lnd_multi-hop_force_close_test.go +++ b/itest/lnd_multi-hop_force_close_test.go @@ -2570,7 +2570,7 @@ func runLocalPreimageClaimLeased(ht *lntest.HarnessTest, // We'll now mine enough blocks so Carol decides that she needs to go // on-chain to claim the HTLC as Bob has been inactive. numBlocks := padCLTV( - invoiceExpiry - ht.CurrentHeight() - incomingBroadcastDelta - 1, + invoiceExpiry - ht.CurrentHeight() - incomingBroadcastDelta, ) ht.MineBlocks(int(numBlocks)) diff --git a/itest/lnd_sweep_test.go b/itest/lnd_sweep_test.go index 8533b74d0..d6e5fcff5 100644 --- a/itest/lnd_sweep_test.go +++ b/itest/lnd_sweep_test.go @@ -1957,7 +1957,7 @@ func runBumpFee(ht *lntest.HarnessTest, alice *node.HarnessNode) { // initial broadcast always succeeds. assertFeeRateGreater(testFeeRate) - // Clean up the mempol. + // Clean up the mempool. ht.MineBlocksAndAssertNumTxes(1, 2) } @@ -2373,6 +2373,6 @@ func testFeeReplacement(ht *lntest.HarnessTest) { uint64(feeRate), 0.02, "want %d, got %d in tx=%v", currentFeeRate, feeRate, outgoingSweep2.TxHash()) - // Finally, clean the mempol. + // Finally, clean the mempool. ht.MineBlocksAndAssertNumTxes(1, 1) } diff --git a/lntest/harness_miner.go b/lntest/harness_miner.go index cde9663c4..010c3f8ca 100644 --- a/lntest/harness_miner.go +++ b/lntest/harness_miner.go @@ -247,7 +247,15 @@ func (h *HarnessTest) GetBestBlock() (*chainhash.Hash, int32) { // MineBlockWithTx mines a single block to include the specifies tx only. func (h *HarnessTest) MineBlockWithTx(tx *wire.MsgTx) *wire.MsgBlock { - return h.miner.MineBlockWithTx(tx) + // Update the harness's current height. + defer h.updateCurrentHeight() + + block := h.miner.MineBlockWithTx(tx) + + // Finally, make sure all the active nodes are synced. + h.AssertActiveNodesSyncedTo(block.BlockHash()) + + return block } // ConnectToMiner connects the miner to a temp miner. From 8ad122bd03d9d28a30e51e02c0e78e11c4ef51a9 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Wed, 19 Mar 2025 17:21:06 +0800 Subject: [PATCH 08/13] itest: add `testBumpFeeLowBudget` --- itest/list_on_test.go | 4 + itest/lnd_sweep_test.go | 175 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 179 insertions(+) diff --git a/itest/list_on_test.go b/itest/list_on_test.go index 139a1917f..168465fb8 100644 --- a/itest/list_on_test.go +++ b/itest/list_on_test.go @@ -690,6 +690,10 @@ var allTestCases = []*lntest.TestCase{ Name: "rbf coop close", TestFunc: testCoopCloseRbf, }, + { + Name: "bump fee low budget", + TestFunc: testBumpFeeLowBudget, + }, } // appendPrefixed is used to add a prefix to each test name in the subtests diff --git a/itest/lnd_sweep_test.go b/itest/lnd_sweep_test.go index d6e5fcff5..d5c16b0bd 100644 --- a/itest/lnd_sweep_test.go +++ b/itest/lnd_sweep_test.go @@ -2376,3 +2376,178 @@ func testFeeReplacement(ht *lntest.HarnessTest) { // Finally, clean the mempool. ht.MineBlocksAndAssertNumTxes(1, 1) } + +// testBumpFeeLowBudget checks that when the requested ideal budget cannot be +// met, the sweeper still sweeps the input with the actual budget. +func testBumpFeeLowBudget(ht *lntest.HarnessTest) { + // Create a new node with a large `maxfeerate` so it's easier to run the + // test. + alice := ht.NewNode("Alice", []string{ + "--sweeper.maxfeerate=10000", + }) + + // Fund Alice 2 UTXOs, each has 100k sats. One of the UTXOs will be used + // to create a tx which she sends some coins to herself. The other will + // be used as the budget when CPFPing the above tx. + coin := btcutil.Amount(100_000) + ht.FundCoins(coin, alice) + ht.FundCoins(coin, alice) + + // Alice sends 50k sats to herself. + tx := ht.SendCoins(alice, alice, coin/2) + txid := tx.TxHash() + + // Get Alice's wallet balance to calculate the fees used in the above + // tx. + resp := alice.RPC.WalletBalance() + + // balance is the expected final balance. Alice's initial balance is + // 200k sats, with 100k sats as the budget for the sweeping tx, which + // means her final balance should be 100k sats minus the mining fees + // used in the above `SendCoins`. + balance := btcutil.Amount( + resp.UnconfirmedBalance + resp.ConfirmedBalance, + ) + fee := coin*2 - balance + ht.Logf("Alice's expected final balance=%v, fee=%v", balance, fee) + + // Alice now tries to bump the first output on this tx. + op := &lnrpc.OutPoint{ + TxidBytes: txid[:], + OutputIndex: uint32(0), + } + value := btcutil.Amount(tx.TxOut[0].Value) + + // assertPendingSweepResp is a helper closure that asserts the response + // from `PendingSweep` RPC is returned with expected values. It also + // returns the sweeping tx for further checks. + assertPendingSweepResp := func(budget uint64, + deadline uint32) *wire.MsgTx { + + // Alice should still have one pending sweep. + pendingSweep := ht.AssertNumPendingSweeps(alice, 1)[0] + + // Validate all fields returned from `PendingSweeps` are as + // expected. + require.Equal(ht, op.TxidBytes, pendingSweep.Outpoint.TxidBytes) + require.Equal(ht, op.OutputIndex, + pendingSweep.Outpoint.OutputIndex) + require.Equal(ht, walletrpc.WitnessType_TAPROOT_PUB_KEY_SPEND, + pendingSweep.WitnessType) + require.EqualValuesf(ht, value, pendingSweep.AmountSat, + "amount not matched: want=%d, got=%d", value, + pendingSweep.AmountSat) + require.True(ht, pendingSweep.Immediate) + + require.EqualValuesf(ht, budget, pendingSweep.Budget, + "budget not matched: want=%d, got=%d", budget, + pendingSweep.Budget) + + // Since the request doesn't specify a deadline, we expect the + // existing deadline to be used. + require.Equalf(ht, deadline, pendingSweep.DeadlineHeight, + "deadline height not matched: want=%d, got=%d", + deadline, pendingSweep.DeadlineHeight) + + // We expect to see Alice's original tx and her CPFP tx in the + // mempool. + txns := ht.GetNumTxsFromMempool(2) + + // Find the sweeping tx - assume it's the first item, if it has + // the same txid as the parent tx, use the second item. + sweepTx := txns[0] + if sweepTx.TxHash() == tx.TxHash() { + sweepTx = txns[1] + } + + return sweepTx + } + + // Use a budget that Alice cannot cover using her wallet UTXOs. + budget := coin * 2 + + // Use a deadlineDelta of 3 such that the fee func is initialized as, + // - starting fee rate: 1 sat/vbyte + // - deadline: 3 + // - budget: 200% of Alice's available funds. + deadlineDelta := 3 + + // First bump request - we expect it to succeed as Alice's current funds + // can cover the fees used here given the position of the fee func is at + // 0. + bumpFeeReq := &walletrpc.BumpFeeRequest{ + Outpoint: op, + Budget: uint64(budget), + Immediate: true, + DeadlineDelta: uint32(deadlineDelta), + } + alice.RPC.BumpFee(bumpFeeReq) + + // Calculate the deadline height. + deadline := ht.CurrentHeight() + uint32(deadlineDelta) + + // Assert the pending sweep is created with the expected values: + // - deadline: 3+current height. + // - budget: 2x the wallet balance. + sweepTx1 := assertPendingSweepResp(uint64(budget), deadline) + + // Mine a block to trigger Alice's sweeper to fee bump the tx. + // + // Second bump request - we expect it to succeed as Alice's current + // funds can cover the fees used here, which is 66.7% of her available + // funds given the position of the fee func is at 1. + ht.MineEmptyBlocks(1) + + // Assert the old sweeping tx has been replaced. + ht.AssertTxNotInMempool(sweepTx1.TxHash()) + + // Assert a new sweeping tx is made. + sweepTx2 := assertPendingSweepResp(uint64(budget), deadline) + + // Mine a block to trigger Alice's sweeper to fee bump the tx. + // + // Third bump request - we expect it to fail as Alice's current funds + // cannot cover the fees now, which is 133.3% of her available funds + // given the position of the fee func is at 2. + ht.MineEmptyBlocks(1) + + // Assert the above sweeping tx is still in the mempool. + ht.AssertTxInMempool(sweepTx2.TxHash()) + + // Fund Alice 200k sats, which will be used to cover the budget. + // + // TODO(yy): We are funding Alice more than enough - at this stage Alice + // has a confirmed UTXO of `coin` amount in her wallet, so ideally we + // should only fund another UTXO of `coin` amount. However, since the + // confirmed wallet UTXO has already been used in sweepTx2, there's no + // easy way to tell her wallet to reuse that UTXO in the upcoming + // sweeping tx. + // To properly fix it, we should provide more granular UTXO management + // here by leveraing `LeaseOutput` - whenever we use a wallet UTXO, we + // should lock it first. And when the sweeping attempt fails, we should + // release it so the UTXO can be used again in another batch. + walletTx := ht.FundCoinsUnconfirmed(coin*2, alice) + + // Mine a block to confirm the above funding coin. + // + // Fourth bump request - we expect it to succeed as Alice's current + // funds can cover the full budget. + ht.MineBlockWithTx(walletTx) + + flakeRaceInBitcoinClientNotifications(ht) + + // Assert Alice's previous sweeping tx has been replaced. + ht.AssertTxNotInMempool(sweepTx2.TxHash()) + + // Assert the pending sweep is created with the expected values: + // - deadline: 3+current height. + // - budget: 2x the wallet balance. + sweepTx3 := assertPendingSweepResp(uint64(budget), deadline) + require.NotEqual(ht, sweepTx2.TxHash(), sweepTx3.TxHash()) + + // Mine the sweeping tx. + ht.MineBlocksAndAssertNumTxes(1, 2) + + // Assert Alice's wallet balance. a + ht.WaitForBalanceConfirmed(alice, balance) +} From 70dec8e169127437ca555b225cc1ea3d65a7ac88 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Fri, 21 Mar 2025 01:29:28 +0800 Subject: [PATCH 09/13] lnd: log sync status in `GetInfo` This is added to fix a flake found in starting the node. --- rpcserver.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/rpcserver.go b/rpcserver.go index 6a3cb9707..f000385c9 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -9056,6 +9056,9 @@ func (r *rpcServer) getChainSyncInfo() (*chainSyncInfo, error) { // Exit early if the wallet is not synced. if !isSynced { + rpcsLog.Debugf("Wallet is not synced to height %v yet", + bestHeight) + return info, nil } @@ -9074,6 +9077,9 @@ func (r *rpcServer) getChainSyncInfo() (*chainSyncInfo, error) { // Exit early if the channel graph is not synced. if !isSynced { + rpcsLog.Debugf("Graph is not synced to height %v yet", + bestHeight) + return info, nil } @@ -9084,5 +9090,10 @@ func (r *rpcServer) getChainSyncInfo() (*chainSyncInfo, error) { // Overwrite isSynced and return. info.isSynced = height == bestHeight + if !info.isSynced { + rpcsLog.Debugf("Blockbeat is not synced to height %v yet", + bestHeight) + } + return info, nil } From 43409c78409bd0b8424fbc9dae004b8ea60e5e66 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Fri, 21 Mar 2025 08:30:06 +0800 Subject: [PATCH 10/13] rpcserver: use `HtlcIndex` as the unique key --- rpcserver.go | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/rpcserver.go b/rpcserver.go index f000385c9..ad0c3392a 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -4841,12 +4841,17 @@ func createRPCOpenChannel(r *rpcServer, dbChannel *channeldb.OpenChannel, ) } - // Create a set of the HTLCs found in the remote commitment, which is + // Create two sets of the HTLCs found in the remote commitment, which is // used to decide whether the HTLCs from the local commitment has been // locked in or not. - remoteHTLCs := fn.NewSet[[32]byte]() + remoteIncomingHTLCs := fn.NewSet[uint64]() + remoteOutgoingHTLCs := fn.NewSet[uint64]() for _, htlc := range dbChannel.RemoteCommitment.Htlcs { - remoteHTLCs.Add(htlc.RHash) + if htlc.Incoming { + remoteIncomingHTLCs.Add(htlc.HtlcIndex) + } else { + remoteOutgoingHTLCs.Add(htlc.HtlcIndex) + } } for i, htlc := range localCommit.Htlcs { @@ -4855,7 +4860,10 @@ func createRPCOpenChannel(r *rpcServer, dbChannel *channeldb.OpenChannel, circuitMap := r.server.htlcSwitch.CircuitLookup() - var forwardingChannel, forwardingHtlcIndex uint64 + var ( + forwardingChannel, forwardingHtlcIndex uint64 + lockedIn bool + ) switch { case htlc.Incoming: circuit := circuitMap.LookupCircuit( @@ -4871,6 +4879,8 @@ func createRPCOpenChannel(r *rpcServer, dbChannel *channeldb.OpenChannel, forwardingHtlcIndex = circuit.Outgoing.HtlcID } + lockedIn = remoteIncomingHTLCs.Contains(htlc.HtlcIndex) + case !htlc.Incoming: circuit := circuitMap.LookupOpenCircuit( htlcswitch.CircuitKey{ @@ -4890,6 +4900,8 @@ func createRPCOpenChannel(r *rpcServer, dbChannel *channeldb.OpenChannel, forwardingHtlcIndex = circuit.Incoming.HtlcID } + + lockedIn = remoteOutgoingHTLCs.Contains(htlc.HtlcIndex) } channel.PendingHtlcs[i] = &lnrpc.HTLC{ @@ -4900,7 +4912,7 @@ func createRPCOpenChannel(r *rpcServer, dbChannel *channeldb.OpenChannel, HtlcIndex: htlc.HtlcIndex, ForwardingChannel: forwardingChannel, ForwardingHtlcIndex: forwardingHtlcIndex, - LockedIn: remoteHTLCs.Contains(rHash), + LockedIn: lockedIn, } // Add the Pending Htlc Amount to UnsettledBalance field. From b6daa3bad4def71d86afce3814ecf88d55c69828 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Fri, 21 Mar 2025 12:11:51 +0800 Subject: [PATCH 11/13] docs: update release notes --- docs/release-notes/release-notes-0.19.0.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/release-notes/release-notes-0.19.0.md b/docs/release-notes/release-notes-0.19.0.md index fd3cb2140..091e92ff3 100644 --- a/docs/release-notes/release-notes-0.19.0.md +++ b/docs/release-notes/release-notes-0.19.0.md @@ -158,6 +158,12 @@ close transaction. tests to set a lower value for faster funding confirmation timeout while keeping the default of 2016 blocks for production stability. +* Previously when sweeping inputs, the sweeper will wait until the specified + budget can be covered by the inputs, which could cause the sweep to be + delayed, and the sweeping tx ends up using large fees. This is now + [changed](https://github.com/lightningnetwork/lnd/pull/9627) so the sweeper + will always attempt the sweep as long as the budget can be partially covered. + ## RPC Additions * [Add a new rpc endpoint](https://github.com/lightningnetwork/lnd/pull/8843) From c7bea07d58c5b458941f4f3abba6f7efdd8c4459 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Tue, 25 Mar 2025 10:28:05 +0800 Subject: [PATCH 12/13] sweep: start the sweeping if there are normal inputs We now start the sweeping process if there are normal inputs to partially cover the budget. --- sweep/tx_input_set.go | 58 ++++++++++++++++++++++++++---- sweep/tx_input_set_test.go | 72 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 7 deletions(-) diff --git a/sweep/tx_input_set.go b/sweep/tx_input_set.go index db4b893ed..ce69a9158 100644 --- a/sweep/tx_input_set.go +++ b/sweep/tx_input_set.go @@ -327,6 +327,21 @@ func (b *BudgetInputSet) copyInputs() []*SweeperInput { return inputs } +// hasNormalInput return a bool to indicate whether there exists an input that +// doesn't require a TxOut. When an input has no required outputs, it's either a +// wallet input, or an input we want to sweep. +func (b *BudgetInputSet) hasNormalInput() bool { + for _, inp := range b.inputs { + if inp.RequiredTxOut() != nil { + continue + } + + return true + } + + return false +} + // AddWalletInputs adds wallet inputs to the set until the specified budget is // met. When sweeping inputs with required outputs, although there's budget // specified, it cannot be directly spent from these required outputs. Instead, @@ -350,11 +365,6 @@ func (b *BudgetInputSet) AddWalletInputs(wallet Wallet) error { return fmt.Errorf("list unspent witness: %w", err) } - // Exit early if there are no wallet UTXOs. - if len(utxos) == 0 { - return fmt.Errorf("%w: empty wallet", ErrNotEnoughInputs) - } - // Sort the UTXOs by putting smaller values at the start of the slice // to avoid locking large UTXO for sweeping. // @@ -377,8 +387,20 @@ func (b *BudgetInputSet) AddWalletInputs(wallet Wallet) error { } } - log.Warn("Not enough wallet UTXOs to cover the budget, sweeping " + - "anyway...") + // Exit if there are no inputs can contribute to the fees. + if !b.hasNormalInput() { + return ErrNotEnoughInputs + } + + // If there's at least one input that can contribute to fees, we allow + // the sweep to continue, even though the full budget can't be met. + // Maybe later more wallet inputs will become available and we can add + // them if needed. + budget := b.Budget() + total, spendable := b.inputAmts() + log.Warnf("Not enough wallet UTXOs: need budget=%v, has spendable=%v, "+ + "total=%v, missing at least %v, sweeping anyway...", budget, + spendable, total, budget-spendable) return nil } @@ -416,6 +438,28 @@ func (b *BudgetInputSet) Inputs() []input.Input { return inputs } +// inputAmts returns two values for the set - the total input amount, and the +// spendable amount. Only the spendable amount can be used to pay the fees. +func (b *BudgetInputSet) inputAmts() (btcutil.Amount, btcutil.Amount) { + var ( + totalAmt btcutil.Amount + spendableAmt btcutil.Amount + ) + + for _, inp := range b.inputs { + output := btcutil.Amount(inp.SignDesc().Output.Value) + totalAmt += output + + if inp.RequiredTxOut() != nil { + continue + } + + spendableAmt += output + } + + return totalAmt, spendableAmt +} + // StartingFeeRate returns the max starting fee rate found in the inputs. // // NOTE: part of the InputSet interface. diff --git a/sweep/tx_input_set_test.go b/sweep/tx_input_set_test.go index 6e91a3fb1..159824878 100644 --- a/sweep/tx_input_set_test.go +++ b/sweep/tx_input_set_test.go @@ -456,6 +456,13 @@ func TestAddWalletInputsNotEnoughInputs(t *testing.T) { mockInput.On("RequiredTxOut").Return(&wire.TxOut{}) defer mockInput.AssertExpectations(t) + sd := &input.SignDescriptor{ + Output: &wire.TxOut{ + Value: budget, + }, + } + mockInput.On("SignDesc").Return(sd).Once() + // Create a pending input that requires 10k satoshis. pi := &SweeperInput{ Input: mockInput, @@ -484,6 +491,71 @@ func TestAddWalletInputsNotEnoughInputs(t *testing.T) { require.Len(t, set.inputs, 2) } +// TestAddWalletInputsEmptyWalletSuccess checks that when the wallet is empty, +// if there is a normal input, no error is returned. +func TestAddWalletInputsEmptyWalletSuccess(t *testing.T) { + t.Parallel() + + wallet := &MockWallet{} + defer wallet.AssertExpectations(t) + + // Specify the min and max confs used in + // ListUnspentWitnessFromDefaultAccount. + minConf, maxConf := int32(1), int32(math.MaxInt32) + + // Assume the desired budget is 10k satoshis. + const budget = 10_000 + + // Create a mock input that has required outputs. + mockInput1 := &input.MockInput{} + defer mockInput1.AssertExpectations(t) + + mockInput1.On("RequiredTxOut").Return(&wire.TxOut{}) + + sd := &input.SignDescriptor{ + Output: &wire.TxOut{ + Value: budget, + }, + } + mockInput1.On("SignDesc").Return(sd).Once() + + // Create a pending input that requires 10k satoshis. + pi1 := &SweeperInput{ + Input: mockInput1, + params: Params{Budget: budget}, + } + + // Create a mock input that doesn't require outputs. + mockInput2 := &input.MockInput{} + defer mockInput2.AssertExpectations(t) + + mockInput2.On("RequiredTxOut").Return(nil) + sd2 := &input.SignDescriptor{ + Output: &wire.TxOut{ + Value: budget, + }, + } + mockInput2.On("SignDesc").Return(sd2).Once() + + // Create a pending input that requires 10k satoshis. + pi2 := &SweeperInput{ + Input: mockInput2, + params: Params{Budget: budget}, + } + + // Mock the wallet to return empty utxos. + wallet.On("ListUnspentWitnessFromDefaultAccount", + minConf, maxConf).Return([]*lnwallet.Utxo{}, nil).Once() + + // Initialize an input set with the pending inputs. + set := BudgetInputSet{inputs: []*SweeperInput{pi1, pi2}} + + // Add wallet inputs to the input set, which should return no error + // although the wallet is empty. + err := set.AddWalletInputs(wallet) + require.NoError(t, err) +} + // TestAddWalletInputsSuccess checks that when there are enough wallet utxos, // they are added to the input set. func TestAddWalletInputsSuccess(t *testing.T) { From ec2f3add6ea680217c16ecccfff950c29eaf6767 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Tue, 25 Mar 2025 10:28:53 +0800 Subject: [PATCH 13/13] sweep: remove dead code --- sweep/tx_input_set.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/sweep/tx_input_set.go b/sweep/tx_input_set.go index ce69a9158..7b533c232 100644 --- a/sweep/tx_input_set.go +++ b/sweep/tx_input_set.go @@ -320,13 +320,6 @@ func (b *BudgetInputSet) NeedWalletInput() bool { return budgetBorrowable < budgetNeeded } -// copyInputs returns a copy of the slice of the inputs in the set. -func (b *BudgetInputSet) copyInputs() []*SweeperInput { - inputs := make([]*SweeperInput, len(b.inputs)) - copy(inputs, b.inputs) - return inputs -} - // hasNormalInput return a bool to indicate whether there exists an input that // doesn't require a TxOut. When an input has no required outputs, it's either a // wallet input, or an input we want to sweep.