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