Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions docs/release-notes/release-notes-0.22.0.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -116,3 +123,4 @@
* bitromortac
* Boris Nagaev
* Erick Cestari
* Jared Tobin
43 changes: 39 additions & 4 deletions sweep/aggregator.go
Original file line number Diff line number Diff line change
Expand Up @@ -232,12 +232,47 @@ 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.
//
// We query per-input rather than once per round: the call
// answers "what does this input contribute to the extra
// budget of any set it joins", and is consistent with the
// once-per-set call at set construction time so long as the
// aux contribution is additive across inputs (which it is for
// the resolution-blob accounting used in practice). 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(
Comment thread
jtobin marked this conversation as resolved.
b.auxSweeper,
func(aux AuxSweeper) fn.Result[btcutil.Amount] {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor F3 · unresolved

Still present: ExtraBudgetForInputs is invoked once per input inside the loop, every sweep round, where the other expensive call (RelayFeePerKW) is hoisted to once per round, and where set construction computes the aux budget once per set. The author declined to change this, reasoning that a per-input filter check needs per-input granularity — fair for correctness, but if the aux implementation does non-trivial work per call (resolution-blob lookup) this remains O(n) aux calls per round. reach=low bounds the blast radius. Left as a documented trade-off; no fix required, but the cost asymmetry stands.

return aux.ExtraBudgetForInputs(
Comment thread
jtobin marked this conversation as resolved.
[]input.Input{pi.Input},
)
Comment thread
jtobin marked this conversation as resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Major F2 · partially_addressed

The new comment documents the intended semantics — ExtraBudgetForInputs([]input.Input{pi.Input}) returns this input's individual contribution, and per-input credits are asserted to sum to the set-level total. That answers the "confirm the semantics" half. What remains: the admission criterion is now only correct if ExtraBudgetForInputs is strictly additive (f({a}) + f({b}) == f({a,b})), and the code neither enforces that nor lets it be checked from the diff — NewBudgetInputSet computes the aux budget once over the whole slice (a single shared pool), while filterInputs sums a per-input query. If any AuxSweeper implementation is sub-additive or capped, the per-input sum over-counts and the filter admits inputs whose constructed set cannot pay aggregate fees, with the divergence surfacing downstream at tx construction rather than here. Codify additivity as a documented contract on the AuxSweeper interface, or query the aggregate over the surviving inputs to mirror NewBudgetInputSet exactly.

},
).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
Comment thread
jtobin marked this conversation as resolved.

// Skip inputs that has too little budget.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor F5: External aux budget added without non-negative or overflow clamp

budget := pi.params.Budget + extraBudget adds an externally-sourced btcutil.Amount (signed int64) from AuxSweeper.ExtraBudgetForInputs with no clamp. A negative return would make budget smaller than pi.params.Budget, tightening the filter below what the input's own budget covers and re-introducing the stranding this PR fixes; a value near MaxInt64 would overflow and wrap negative, silently dropping the input. The realistic resolution-blob accounting returns non-negative amounts and the implementation is out of tree, so reachability can't be confirmed from the diff — hence minor/defense-in-depth. Clamping extraBudget to >= 0 (and using a saturating add) keeps this guard strictly relaxing the filter, never tightening it relative to the input's own 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
Expand All @@ -248,10 +283,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
Expand Down
102 changes: 102 additions & 0 deletions sweep/aggregator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,108 @@
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 own-budget-sufficient input",

Check failure on line 206 in sweep/aggregator_test.go

View workflow job for this annotation

GitHub Actions / Lint code

the line is 82 characters long, which exceeds the maximum of 80 characters. (ll)
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 {
tc := tc

Check failure on line 222 in sweep/aggregator_test.go

View workflow job for this annotation

GitHub Actions / Lint code

The copy of the 'for' variable "tc" can be deleted (Go 1.22+) (copyloopvar)
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)
if tc.expectKept {
mockInput.On("RequiredTxOut").Return(nil)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor F6: Conditional RequiredTxOut stub masks regressions as mock panics

In TestBudgetAggregatorFilterInputsAuxBudget the mockInput.On("RequiredTxOut").Return(nil) stub is registered only when tc.expectKept is true. The aux-error-drops case (expectKept: false) is correct only if the min-fee guard's continue fires before the RequiredTxOut dust check is reached. If a regression let that case fall through to the dust check, the test would panic on the unstubbed mock instead of failing with a clear assertion — coupling the test's failure mode to the exact control flow it is meant to verify. Register the RequiredTxOut stub unconditionally so a regression surfaces as a clean assertion failure.

}

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) {
Expand Down
Loading