Merge pull request #10897 from jtobin/htlc-force-close-sweeper

sweep: account for aux extra budget when filtering inputs

docs/release-notes/release-notes-0.22.0.md

@@ -42,6 +42,13 @@
   regardless of peer connectivity. Uptime is now seeded from the peer's
   actual connection state.
 
+* [Fixed a bug](https://github.com/lightningnetwork/lnd/pull/10897) in the
+  sweeper whereby inputs that receive an extra budget from an aux sweeper
+  (such as custom channel outputs, whose value is mostly carried off-chain)
+  were filtered against their own budget alone. This could permanently
+  exclude such inputs from sweeping even though their input set could
+  comfortably pay its fees.
+
 # New Features
 
 ## Functional Enhancements
@@ -134,3 +141,4 @@
 * bitromortac
 * Boris Nagaev
 * Erick Cestari
+* Jared Tobin

sweep/aggregator.go

@@ -232,12 +232,46 @@ func (b *BudgetAggregator) filterInputs(inputs InputsMap) InputsMap {
 		// https://github.com/lightning/bolts/blob/master/03-transactions.md#appendix-a-expected-weights
 		wu := lntypes.VByte(input.InputSize).ToWU() + witnessSize
 
+		// If an aux sweeper is set, it may contribute an extra budget
+		// to any input set this input becomes part of. The input's own
+		// budget may be tiny (e.g. for custom channel outputs whose
+		// value is mostly carried off-chain), so without accounting
+		// for the extra budget here we'd filter such inputs out
+		// permanently, even though their input set could comfortably
+		// pay its fees.
+		//
+		// The AuxSweeper interface requires the contribution to be
+		// non-negative and additive across inputs, so a singleton
+		// call returns this input's share and per-input credits sum
+		// to the set-level total used at set construction. On a
+		// lookup error we fall back to zero extra budget rather than
+		// dropping the input, so a transient aux failure doesn't
+		// recreate the silently-stranded mode this guard is meant to
+		// avoid.
+		extraBudget, err := fn.MapOptionZ(
+			b.auxSweeper,
+			func(aux AuxSweeper) fn.Result[btcutil.Amount] {
+				return aux.ExtraBudgetForInputs(
+					[]input.Input{pi.Input},
+				)
+			},
+		).Unpack()
+		if err != nil {
+			log.Errorf("Unable to fetch extra budget for "+
+				"input=%v, falling back to own budget: %v",
+				op, err)
+
+			extraBudget = 0
+		}
+
+		budget := pi.params.Budget + extraBudget
+
 		// Skip inputs that has too little budget.
 		minFee := minFeeRate.FeeForWeight(wu)
-		if pi.params.Budget < minFee {
+		if budget < minFee {
 			log.Warnf("Skipped input=%v: has budget=%v, but the "+
 				"min fee requires %v (feerate=%v), size=%v", op,
-				pi.params.Budget, minFee,
+				budget, minFee,
 				minFeeRate.FeePerVByte(), wu.ToVB())
 
 			continue
@@ -248,10 +282,10 @@ func (b *BudgetAggregator) filterInputs(inputs InputsMap) InputsMap {
 			chainfee.SatPerKWeight(0),
 		)
 		startingFee := startingFeeRate.FeeForWeight(wu)
-		if pi.params.Budget < startingFee {
+		if budget < startingFee {
 			log.Errorf("Skipped input=%v: has budget=%v, but the "+
 				"starting fee requires %v (feerate=%v), "+
-				"size=%v", op, pi.params.Budget, startingFee,
+				"size=%v", op, budget, startingFee,
 				startingFeeRate.FeePerVByte(), wu.ToVB())
 
 			continue

sweep/aggregator_test.go

@@ -164,6 +164,112 @@ func TestBudgetAggregatorFilterInputs(t *testing.T) {
 	require.Contains(t, result, opHigh)
 }
 
+// TestBudgetAggregatorFilterInputsAuxBudget checks that the aux sweeper's
+// extra budget is folded into the filter's budget check, and that an aux
+// lookup failure falls back to gating on the input's own budget rather than
+// silently dropping the input.
+func TestBudgetAggregatorFilterInputsAuxBudget(t *testing.T) {
+	t.Parallel()
+
+	const wu lntypes.WeightUnit = 100
+	inpSize := lntypes.VByte(input.InputSize).ToWU() + wu
+
+	const minFeeRate = chainfee.SatPerKWeight(1000)
+	minFee := minFeeRate.FeeForWeight(inpSize)
+
+	// shortfall is how much the own budget falls short of minFee; the aux
+	// sweeper covers exactly this gap in the "rescue" cases.
+	const shortfall = btcutil.Amount(100)
+	auxErr := errors.New("aux failure")
+
+	testCases := []struct {
+		name       string
+		ownBudget  btcutil.Amount
+		auxResult  fn.Result[btcutil.Amount]
+		expectKept bool
+	}{
+		{
+			// The input's own budget falls short of the min fee,
+			// but the aux sweeper contributes enough extra budget
+			// to clear it. Pre-fix this input would have been
+			// filtered out.
+			name:       "aux budget rescues low-own-budget input",
+			ownBudget:  minFee - shortfall,
+			auxResult:  fn.Ok(shortfall),
+			expectKept: true,
+		},
+		{
+			// The aux lookup errors but the input's own budget
+			// already covers the min fee, so the conservative
+			// fallback (extraBudget=0) keeps it in. Pre-fix this
+			// input would have been silently dropped.
+			name:       "aux error keeps sufficient input",
+			ownBudget:  minFee,
+			auxResult:  fn.Err[btcutil.Amount](auxErr),
+			expectKept: true,
+		},
+		{
+			// The aux lookup errors and the input cannot pay its
+			// own way, so it is correctly filtered.
+			name:       "aux error drops below-min-fee input",
+			ownBudget:  minFee - shortfall,
+			auxResult:  fn.Err[btcutil.Amount](auxErr),
+			expectKept: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			estimator := &chainfee.MockEstimator{}
+			defer estimator.AssertExpectations(t)
+			estimator.On("RelayFeePerKW").Return(minFeeRate).Once()
+
+			wt := &input.MockWitnessType{}
+			defer wt.AssertExpectations(t)
+			wt.On("SizeUpperBound").Return(wu, true, nil).Once()
+
+			mockInput := &input.MockInput{}
+			defer mockInput.AssertExpectations(t)
+			op := wire.OutPoint{Hash: chainhash.Hash{1}}
+			mockInput.On("WitnessType").Return(wt)
+			mockInput.On("OutPoint").Return(op)
+
+			// Stub RequiredTxOut unconditionally so a regression
+			// that lets the dropped case fall through to the dust
+			// check surfaces as a clean assertion failure rather
+			// than an unstubbed-mock panic. `Maybe()` is needed
+			// because the dropped case shouldn't actually reach
+			// this call.
+			mockInput.On("RequiredTxOut").Return(nil).Maybe()
+
+			mockAux := &MockAuxSweeper{}
+			defer mockAux.AssertExpectations(t)
+			mockAux.On("ExtraBudgetForInputs").Return(tc.auxResult)
+
+			inputs := InputsMap{
+				op: &SweeperInput{
+					Input:  mockInput,
+					params: Params{Budget: tc.ownBudget},
+				},
+			}
+
+			b := NewBudgetAggregator(
+				estimator, 0,
+				fn.Some[AuxSweeper](mockAux),
+			)
+			result := b.filterInputs(inputs)
+
+			if tc.expectKept {
+				require.Contains(t, result, op)
+			} else {
+				require.NotContains(t, result, op)
+			}
+		})
+	}
+}
+
 // TestBudgetAggregatorSortInputs checks that inputs are sorted by based on
 // their budgets and force flag.
 func TestBudgetAggregatorSortInputs(t *testing.T) {

sweep/interface.go

@@ -88,6 +88,13 @@ type AuxSweeper interface {
 	// should be allocated to sweep the given set of inputs. This can be
 	// used to add extra funds to the sweep transaction, for example to
 	// cover fees for additional outputs of custom channels.
+	//
+	// The returned amount must be non-negative, and the contribution
+	// must be additive across inputs: the result for a slice of inputs
+	// must equal the sum of the per-input results, so that callers may
+	// query the contribution of a single input by passing a singleton
+	// slice. The budget aggregator relies on this when pre-filtering
+	// inputs by their own budget plus their individual aux contribution.
 	ExtraBudgetForInputs(inputs []input.Input) fn.Result[btcutil.Amount]
 
 	// NotifyBroadcast is used to notify external callers of the broadcast