From 6b59a540f1ecfb85ce4ad2721b38885e02a78ef3 Mon Sep 17 00:00:00 2001 From: Jared Tobin Date: Mon, 18 May 2026 20:22:38 -0230 Subject: [PATCH 1/2] sweep: don't ratchet starting fee rate on non-fee failures When a sweep fails to publish, the sweeper computes a higher retry fee rate and stores it as the input's new starting rate. That's the right move for fee-related failures, where the broadcast was rejected for not paying enough, but the same machinery also fires on resource failures (no wallet UTXO available, the input set's budget exceeded), which carry no fee-rate signal. Repeatedly ratcheting the rate on these can push it past an input's intrinsic budget, after which the aggregator silently skips the input and the sweep is stranded forever. Distinguish fee-rate-bearing failures from resource ones at the point where the bump result is produced, and skip the rate ratchet for the latter: the input's existing starting fee rate is preserved across non-fee failures, so the next retry starts at the rate the original sweep request specified. --- sweep/fee_bumper.go | 134 ++++++++++++++++++++++------------ sweep/fee_bumper_test.go | 153 ++++++++++++++++++++++++++++++++++++--- sweep/sweeper.go | 28 ++++++- sweep/sweeper_test.go | 42 ++++++++++- 4 files changed, 298 insertions(+), 59 deletions(-) diff --git a/sweep/fee_bumper.go b/sweep/fee_bumper.go index 6088094ffe8..b81409b4c8c 100644 --- a/sweep/fee_bumper.go +++ b/sweep/fee_bumper.go @@ -271,6 +271,16 @@ type BumpResult struct { // FeeRate is the fee rate used for the new tx. FeeRate chainfee.SatPerKWeight + // NextStartingFeeRate, when set, is the fee rate the sweeper should + // use as the starting rate for the next attempt to sweep this input + // set. It is only meaningful on failure-class events (TxFailed, + // TxUnknownSpend). fn.None signals that the input's existing + // starting fee rate must be preserved, and is used for any + // failure path that carries no fee-rate signal: the resource + // failures handled by isResourceFailure, but also non-fee initial + // errors such as ErrTxNoOutput and ErrZeroFeeRateDelta. + NextStartingFeeRate fn.Option[chainfee.SatPerKWeight] + // Fee is the fee paid by the new tx. Fee btcutil.Amount @@ -769,15 +779,23 @@ func (t *TxPublisher) broadcast(record *monitorRecord) (*BumpResult, error) { event = TxFailed } + feeRate := record.feeFunction.FeeRate() result := &BumpResult{ Event: event, Tx: record.tx, Fee: record.fee, - FeeRate: record.feeFunction.FeeRate(), + FeeRate: feeRate, Err: err, requestID: record.requestID, } + // On a wallet-publish failure, carry the attempted fee rate as the + // next starting fee rate so the sweeper resumes from where we left + // off rather than the input's original starting rate. + if event == TxFailed { + result.NextStartingFeeRate = fn.Some(feeRate) + } + return result, nil } @@ -1090,6 +1108,17 @@ func (t *TxPublisher) handleTxConfirmed(r *monitorRecord) { t.handleResult(result) } +// isResourceFailure reports whether err describes a resource-level failure +// (insufficient wallet inputs or insufficient budget) rather than a +// fee-related failure. These failures carry no fee-rate signal, so the +// fee bumper must not ratchet the input's starting fee rate when handling +// them: doing so can permanently strand an input whose intrinsic budget +// cannot accommodate a higher starting rate. +func isResourceFailure(err error) bool { + return errors.Is(err, ErrNotEnoughInputs) || + errors.Is(err, ErrNotEnoughBudget) +} + // handleInitialTxError takes the error from `initializeTx` and decides the // bump event. It will construct a BumpResult and handles it. func (t *TxPublisher) handleInitialTxError(r *monitorRecord, err error) { @@ -1112,30 +1141,37 @@ func (t *TxPublisher) handleInitialTxError(r *monitorRecord, err error) { case errors.Is(err, ErrZeroFeeRateDelta): 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. + // When the linear fee function has exhausted its position window + // (i.e. the deadline has effectively elapsed without confirmation), + // we'll send a TxFailed so these inputs can be retried with a + // different group in the next block. This is still a fee-related + // failure mode, so we calculate a retry fee rate for the next + // attempt. 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. + // Calculate the starting fee rate to be used when retrying + // to sweep these inputs, and carry it on the result so the + // sweeper picks it up as the starting rate. feeRate, err := t.calculateRetryFeeRate(r) if err != nil { result.Event = TxFatal result.Err = err + + break } - // Attach the new fee rate. - result.FeeRate = feeRate + result.NextStartingFeeRate = fn.Some(feeRate) + + // For resource failures (see isResourceFailure) we return a + // TxFailed event so the sweeper can re-cluster and retry on the + // next block, but we intentionally do NOT call calculateRetryFeeRate + // and leave NextStartingFeeRate as fn.None. That signals the + // sweeper to preserve the input's existing starting fee rate; + // otherwise repeated resource failures would ratchet the rate past + // the input's intrinsic budget and strand it. + case isResourceFailure(err): + 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. @@ -1282,17 +1318,19 @@ func (t *TxPublisher) createUnknownSpentBumpResult( SpentInputs: r.spentInputs, } - // Calculate the next fee rate for the retry. + // Calculate the next fee rate for the retry and attach it as the + // next starting fee rate for the sweeper. feeRate, err := t.calculateRetryFeeRate(r) if err != nil { // Overwrite the event and error so the sweeper will // remove this input. result.Event = TxFatal result.Err = err + + return result } - // Attach the new fee rate to be used for the next sweeping attempt. - result.FeeRate = feeRate + result.NextStartingFeeRate = fn.Some(feeRate) return result } @@ -1851,48 +1889,52 @@ func (t *TxPublisher) handleReplacementTxError(r *monitorRecord, // been spent by another tx and confirmed. In this case we will handle // it by returning a TxUnknownSpend bump result. if errors.Is(err, ErrInputMissing) { - log.Warnf("Fail to fee bump tx %v: %v", oldTx.TxHash(), err) + log.Warnf("Failed to fee bump tx %v: %v", oldTx.TxHash(), err) bumpResult := t.handleMissingInputs(r) return fn.Some(*bumpResult) } - // 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 + // For resource failures (see isResourceFailure) we return a TxFailed + // event with no retry fee rate; the sweeper will re-cluster and + // retry on the next block, preserving the input's existing starting + // fee rate. + if isResourceFailure(err) { + log.Warnf("Failed to fee bump tx %v: %v", oldTx.TxHash(), err) + return fn.Some(BumpResult{ + Event: TxFailed, + Tx: oldTx, + Err: err, + requestID: r.requestID, + }) } - // If the error is not fee related, we will return a `TxFailed` event so - // this input can be retried. - result := fn.Some(BumpResult{ - Event: event, + // For all other fee-bump failures, treat them as fee-related: ask + // the fee function for the next rate and carry it on the result as + // the starting fee rate for the sweeper's next attempt. + bumpResult := BumpResult{ + Event: TxFailed, Tx: oldTx, Err: err, requestID: r.requestID, - FeeRate: feeRate, - }) - - // 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) || - errors.Is(err, ErrNotEnoughInputs) { + } - log.Warnf("Fail to fee bump tx %v: %v", oldTx.TxHash(), err) - return result + feeRate, ferr := t.calculateRetryFeeRate(r) + if ferr != nil { + // If there's an error with the fee calculation, we need to + // abort the sweep. Attribute the fatal transition to ferr so + // operators see the actual cause, matching the sibling paths + // in handleInitialTxError and createUnknownSpentBumpResult. + bumpResult.Event = TxFatal + bumpResult.Err = ferr + } else { + bumpResult.NextStartingFeeRate = fn.Some(feeRate) } - // Otherwise, an unexpected error occurred, we will log an error and let - // the sweeper retry the whole process. + // Log an error and let the sweeper retry the whole process. log.Errorf("Failed to bump tx %v: %v", oldTx.TxHash(), err) - return result + return fn.Some(bumpResult) } // calculateRetryFeeRate calculates a new fee rate to be used as the starting diff --git a/sweep/fee_bumper_test.go b/sweep/fee_bumper_test.go index c2653d795c2..545d9ef14a8 100644 --- a/sweep/fee_bumper_test.go +++ b/sweep/fee_bumper_test.go @@ -766,12 +766,13 @@ func TestTxPublisherBroadcast(t *testing.T) { }, expectedErr: nil, expectedResult: &BumpResult{ - Event: TxFailed, - Tx: tx, - Fee: fee, - FeeRate: feerate, - Err: errDummy, - requestID: requestID, + Event: TxFailed, + Tx: tx, + Fee: fee, + FeeRate: feerate, + NextStartingFeeRate: fn.Some(feerate), + Err: errDummy, + requestID: requestID, }, }, { @@ -1130,7 +1131,14 @@ func TestCreateAndPublishFail(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() + + // Note: we deliberately do not expect Increment() to be called for + // ErrNotEnoughBudget. Ratcheting the starting fee rate on a non-fee + // failure mode would only strand inputs whose intrinsic budget can't + // accommodate a higher rate; handleReplacementTxError (and the + // matching path in handleInitialTxError) skips the fee-function + // increment for ErrNotEnoughBudget and ErrNotEnoughInputs precisely + // for that reason. // Create a testing monitor record. req := createTestBumpRequest() @@ -1163,6 +1171,11 @@ func TestCreateAndPublishFail(t *testing.T) { require.ErrorIs(t, result.Err, ErrNotEnoughBudget) require.Equal(t, requestID, result.requestID) + // The result must NOT carry a next starting fee rate; + // ErrNotEnoughBudget is not a fee-related failure and ratcheting + // would be wrong. + require.True(t, result.NextStartingFeeRate.IsNone()) + // Increase the budget and call it again. This time we will mock an // error to be returned from CheckMempoolAcceptance. req.Budget = 1000 @@ -1794,8 +1807,9 @@ func TestProcessRecordsSpent(t *testing.T) { require.Equal(t, TxUnknownSpend, result.Event) require.Equal(t, tx, result.Tx) - // We expect the fee rate to be updated. - require.Equal(t, feerate, result.FeeRate) + // We expect the next starting fee rate to be carried for the + // sweeper's retry. + require.Equal(t, fn.Some(feerate), result.NextStartingFeeRate) // No error should be set. require.ErrorIs(t, result.Err, ErrUnknownSpent) @@ -1983,6 +1997,127 @@ func TestHandleInitialBroadcastFail(t *testing.T) { require.Equal(t, 0, tp.subscriberChans.Len()) } +// TestHandleInitialTxErrorNoRatchetOnResourceFailure asserts that +// handleInitialTxError does not call the fee function's Increment method +// when the failure is ErrNotEnoughInputs or ErrNotEnoughBudget. Those +// failure modes carry no fee-rate signal; ratcheting the rate on each +// retry would only strand inputs whose intrinsic budget cannot accommodate +// the increased starting rate. +func TestHandleInitialTxErrorNoRatchetOnResourceFailure(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + err error + }{ + {"ErrNotEnoughInputs", ErrNotEnoughInputs}, + {"ErrNotEnoughBudget", ErrNotEnoughBudget}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + tp, _ := createTestPublisher(t) + + // We intentionally do not stub Increment() on the + // mock fee function: if the code under test calls + // it on a resource failure, testify will panic and + // the test will fail loudly. + + inp := createTestInput(1000, input.WitnessKeyHash) + req := &BumpRequest{ + DeliveryAddress: changePkScript, + Inputs: []input.Input{&inp}, + Budget: btcutil.Amount(1000), + MaxFeeRate: chainfee.SatPerKWeight(10000), + DeadlineHeight: 10, + } + + rec := &monitorRecord{ + requestID: 1, + req: req, + } + + // Subscribe so we can observe the bump result. + subChan := make(chan *BumpResult, 1) + tp.subscriberChans.Store(uint64(1), subChan) + + tp.handleInitialTxError(rec, tc.err) + + select { + case result := <-subChan: + require.Equal(t, TxFailed, result.Event) + require.ErrorIs(t, result.Err, tc.err) + require.True( + t, + result.NextStartingFeeRate.IsNone(), + "NextStartingFeeRate must be None "+ + "for resource failures", + ) + case <-time.After(time.Second): + t.Fatal("timeout waiting for result") + } + }) + } +} + +// TestHandleInitialTxErrorRatchetsOnErrMaxPosition asserts that +// handleInitialTxError still calls calculateRetryFeeRate (and so the fee +// function's Increment method) on an ErrMaxPosition failure, and that the +// resulting BumpResult carries a Some next starting fee rate for the +// sweeper. This locks in the disjointness between ErrMaxPosition (a +// fee-related failure that should ratchet) and the resource failures +// covered by TestHandleInitialTxErrorNoRatchetOnResourceFailure (which +// must not ratchet). +func TestHandleInitialTxErrorRatchetsOnErrMaxPosition(t *testing.T) { + t.Parallel() + + tp, m := createTestPublisher(t) + + // Stub Increment and FeeRate so calculateRetryFeeRate returns a + // non-zero rate. If handleInitialTxError ever stops calling + // Increment for ErrMaxPosition, testify will panic on the + // unexpected absence and fail the test. + retryRate := chainfee.SatPerKWeight(5000) + m.feeFunc.On("Increment").Return(true, nil).Once() + m.feeFunc.On("FeeRate").Return(retryRate) + + inp := createTestInput(1000, input.WitnessKeyHash) + req := &BumpRequest{ + DeliveryAddress: changePkScript, + Inputs: []input.Input{&inp}, + Budget: btcutil.Amount(1000), + MaxFeeRate: chainfee.SatPerKWeight(10000), + DeadlineHeight: 10, + } + + rec := &monitorRecord{ + requestID: 1, + req: req, + feeFunction: m.feeFunc, + } + + // Subscribe so we can observe the bump result. + subChan := make(chan *BumpResult, 1) + tp.subscriberChans.Store(uint64(1), subChan) + + tp.handleInitialTxError(rec, ErrMaxPosition) + + select { + case result := <-subChan: + require.Equal(t, TxFailed, result.Event) + require.ErrorIs(t, result.Err, ErrMaxPosition) + require.Equal( + t, fn.Some(retryRate), result.NextStartingFeeRate, + "ErrMaxPosition must carry a Some next starting "+ + "fee rate so the sweeper ratchets", + ) + case <-time.After(time.Second): + t.Fatal("timeout waiting for result") + } +} + // TestHasInputsSpent checks the expected outpoint:tx map is returned. func TestHasInputsSpent(t *testing.T) { t.Parallel() diff --git a/sweep/sweeper.go b/sweep/sweeper.go index 547a92c47fb..3312dbe92dc 100644 --- a/sweep/sweeper.go +++ b/sweep/sweeper.go @@ -964,8 +964,13 @@ func (s *UtxoSweeper) markInputsPublished(tr *TxRecord, set InputSet) error { } // markInputsPublishFailed marks the list of inputs as failed to be published. +// If nextStartingFeeRate is Some, it is recorded as the input's new starting +// fee rate for the next attempt; if None, the input's existing StartingFeeRate +// is preserved. None applies to any failure path that carries no fee-rate +// signal, which covers the resource failures handled by isResourceFailure as +// well as non-fee initial errors such as ErrTxNoOutput and ErrZeroFeeRateDelta. func (s *UtxoSweeper) markInputsPublishFailed(set InputSet, - feeRate chainfee.SatPerKWeight) { + nextStartingFeeRate fn.Option[chainfee.SatPerKWeight]) { // Reschedule sweep. for _, inp := range set.Inputs() { @@ -994,6 +999,23 @@ func (s *UtxoSweeper) markInputsPublishFailed(set InputSet, // Update the input's state. pi.state = PublishFailed + // Only ratchet the starting fee rate when the BumpResult + // carries one. Any failure path with no fee-rate signal + // (resource failures, dust outputs, zero fee-rate delta, + // etc.) delivers fn.None; preserving the input's existing + // starting rate in that case avoids stranding inputs whose + // intrinsic budget can't accommodate a higher rate and + // avoids resetting the rate to a meaningless value on + // non-fee failures. + if nextStartingFeeRate.IsNone() { + log.Debugf("Input(%v): preserving starting fee rate "+ + "%v across non-fee failure", op, + pi.params.StartingFeeRate) + + continue + } + + feeRate := nextStartingFeeRate.UnsafeFromSome() log.Debugf("Input(%v): updating params: starting fee rate "+ "[%v -> %v]", op, pi.params.StartingFeeRate, feeRate) @@ -1719,7 +1741,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, resp.result.FeeRate) + s.markInputsPublishFailed(resp.set, resp.result.NextStartingFeeRate) } // handleBumpEventTxReplaced handles the case where the sweeping tx has been @@ -1970,7 +1992,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, r.result.FeeRate) + s.markInputsPublishFailed(r.set, r.result.NextStartingFeeRate) // Get all the inputs that are not spent in the current sweeping tx. spentInputs := r.result.SpentInputs diff --git a/sweep/sweeper_test.go b/sweep/sweeper_test.go index 0d22a6dd7b0..e9edbfed13d 100644 --- a/sweep/sweeper_test.go +++ b/sweep/sweeper_test.go @@ -237,7 +237,7 @@ func TestMarkInputsPublishFailed(t *testing.T) { // 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, feeRate) + s.markInputsPublishFailed(set, fn.Some(feeRate)) // We expect unchanged number of pending inputs. require.Len(s.inputs, 7) @@ -282,6 +282,46 @@ func TestMarkInputsPublishFailed(t *testing.T) { mockStore.AssertExpectations(t) } +// TestMarkInputsPublishFailedPreservesRateOnNone asserts that when +// markInputsPublishFailed is invoked with fn.None, the input's existing +// StartingFeeRate is preserved instead of being overwritten. This is the +// sweeper-side contract that resource-failure callers (handleInitialTxError +// and handleReplacementTxError for ErrNotEnoughInputs / ErrNotEnoughBudget) +// rely on: those failures carry no fee-rate signal, so ratcheting the +// starting rate upward on each retry would be incorrect. +func TestMarkInputsPublishFailedPreservesRateOnNone(t *testing.T) { + t.Parallel() + + mockStore := NewMockSweeperStore() + s := New(&UtxoSweeperConfig{Store: mockStore}) + + // Stage an input in PendingPublish with an existing starting fee + // rate. The rate we set here must not be touched by the call below. + existingRate := chainfee.SatPerKWeight(2500) + inp := createMockInput(t, s, PendingPublish) + s.inputs[inp.OutPoint()].params.StartingFeeRate = + fn.Some(existingRate) + + set := &MockInputSet{} + defer set.AssertExpectations(t) + set.On("Inputs").Return([]input.Input{inp}) + + // Mark the input as publish-failed with fn.None, signalling a + // non-fee failure (e.g. ErrNotEnoughInputs). + s.markInputsPublishFailed(set, fn.None[chainfee.SatPerKWeight]()) + + // The input must still be in PublishFailed (state transition is + // independent of the rate signal) but its StartingFeeRate must + // remain the value we set above, not Some(0). + pi := s.inputs[inp.OutPoint()] + require.Equal(t, PublishFailed, pi.state) + require.True(t, pi.params.StartingFeeRate.IsSome()) + require.Equal(t, existingRate, + pi.params.StartingFeeRate.UnsafeFromSome()) + + mockStore.AssertExpectations(t) +} + // TestMarkInputsSwept checks that given a list of inputs with different // states, only the non-terminal state will be marked as `Swept`. func TestMarkInputsSwept(t *testing.T) { From 2fb832579e61d2856f415dc74ef2189487775bd9 Mon Sep 17 00:00:00 2001 From: Jared Tobin Date: Thu, 11 Jun 2026 13:17:19 -0230 Subject: [PATCH 2/2] docs: add release note --- docs/release-notes/release-notes-0.22.0.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/release-notes/release-notes-0.22.0.md b/docs/release-notes/release-notes-0.22.0.md index 2f7f0ca1b7e..2166fafef1e 100644 --- a/docs/release-notes/release-notes-0.22.0.md +++ b/docs/release-notes/release-notes-0.22.0.md @@ -42,6 +42,12 @@ regardless of peer connectivity. Uptime is now seeded from the peer's actual connection state. +* [Fixed a bug](https://github.com/lightningnetwork/lnd/pull/10898) in the + sweeper whereby an input's starting fee rate was ratcheted upward on + failures that carry no fee-rate signal (e.g. a lack of spendable wallet + UTXOs). Repeated spurious ratcheting could push the rate past the input's + budget, after which the sweep was silently stranded. + # New Features ## Functional Enhancements @@ -116,3 +122,4 @@ * bitromortac * Boris Nagaev * Erick Cestari +* Jared Tobin