sweep: account for aux extra budget when filtering inputs#10897
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an accounting issue in the BudgetAggregator that caused certain inputs to be permanently filtered out of sweep attempts. By failing to account for the extra budget provided by an auxiliary sweeper, the system would incorrectly deem small asset-based inputs as insufficient to cover fees, especially after failed attempts ratcheted up the required starting fee rate. This change ensures the filter correctly considers the total available budget, maintaining consistency with how input sets are formed. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the budget aggregator in sweep/aggregator.go to account for an extra budget contributed by an auxiliary sweeper when filtering inputs. Feedback suggests passing the underlying input.Input interface (pi.Input) instead of the wrapper pi (*SweeperInput) to ExtraBudgetForInputs to avoid type assertion failures in external implementations like Taproot Assets.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (1 file)
AnalysisThis PR modifies The change is small (1 file, 30 lines), so no severity bump was applied. To override, add a |
jtobin
force-pushed
the
htlc-force-close-sweeper
branch
from
6a3e5eb to
b77026e
Compare
jtobin
force-pushed
the
htlc-force-close-sweeper
branch
from
b77026e to
75433bb
Compare
|
@jtobin, remember to re-request review from reviewers when ready |
|
/gateway review |
|
✅ Review posted: #10897 (review) 4 finding(s); 4 inline, 0 in body. 🔁 Need a re-review after pushing changes? Reply with |
![lightninglabs-gateway[bot]](https://avatars.githubusercontent.com/in/3635431?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR fixes a real accounting bug: filterInputs previously gated each input on its own budget alone, ignoring the extra budget an aux sweeper contributes to the set the input joins, which could permanently strand small custom-channel-output sweeps after fee-rate ratcheting. The core fix — folding extraBudget into the minFee and startingFee comparisons — is sound and well-motivated.
Two concerns warrant attention before merge. First, the new aux-budget lookup's error path does continue, which silently drops the input from the round — a new exclusion mode that did not exist when these guards were purely economic field reads, and one that recreates the "silently stranded" failure shape the PR sets out to eliminate. Second, the extra budget is fetched per input over a singleton ([]input.Input{pi.Input}) and added to each input's own budget, whereas the budget input set accounts for it once per set; whether per-input crediting of a set-level pool is correct depends on ExtraBudgetForInputs semantics I cannot see in the diff.
Findings: 🔴 0 Blocker · 🟠 2 Major · 🟡 2 Minor · 🔵 0 Nit
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
jtobin
force-pushed
the
htlc-force-close-sweeper
branch
from
75433bb to
73d9d17
Compare
|
/gateway re-review |
|
Sorry, |
|
/gateway re-review |
|
✅ Re-review posted: #10897 (review) Prior findings: 2 addressed, 2 still unresolved, 0 withdrawn. 🔁 Need another re-review after pushing changes? Reply with |
There was a problem hiding this comment.
The two concerns that asked for concrete code changes are resolved: the aux-error path now falls back to extraBudget = 0 instead of dropping the input (prior F1), and TestBudgetAggregatorFilterInputsAuxBudget now exercises the rescue, error-keeps, and error-drops cases (prior F4).
The singleton-vs-set accounting question (prior F2) is only partially resolved. The added comment documents that per-input crediting is correct "so long as the aux contribution is additive," but the code still relies on that property of an out-of-tree AuxSweeper without enforcing it or making it verifiable from the diff — three independent review passes re-raised the same divergence. The per-round-cost observation (prior F3) is unchanged; the author declined to batch, with a stated rationale. Two new minor concerns: the externally-sourced extra budget is added without a non-negative/overflow clamp, and one test stub is set conditionally in a way that turns a regression into a mock panic rather than a clean assertion.
Findings: 🔴 0 Blocker · 🟠 1 Major · 🟡 3 Minor · 🔵 0 Nit
Status of prior findings
- F1 addressed: Fixed in sweep/aggregator.go:268 — the aux-lookup error branch now logs and sets
extraBudget = 0, then falls through tobudget := pi.params.Budget + extraBudget, so a transient aux failure gates the input on its own budget rather than evicting it viacontinue. This is the conservative behavior the finding requested. - F4 addressed: Covered by
TestBudgetAggregatorFilterInputsAuxBudgetin sweep/aggregator_test.go, which exercises (a) aux budget rescuing a low-own-budget input, (b) an aux error keeping an own-budget-sufficient input via the zero fallback, and (c) an aux error still dropping a below-min-fee input. This locks in both the fix and the fallback path.
jtobin
force-pushed
the
htlc-force-close-sweeper
branch
from
73d9d17 to
b4ec666
Compare
|
@saubyk One more should do it. 🤞 |
|
/gateway re-review |
|
✅ Re-review posted: #10897 (review) Prior findings: 4 addressed, 2 still unresolved, 0 withdrawn. 🔁 Need another re-review after pushing changes? Reply with |
There was a problem hiding this comment.
This revision cleanly addresses four of the six prior findings: the aux error path now falls back to a zero extra budget instead of dropping the input (F1), the negative-clamp and saturating add are in place (F5), the conditional test stub is now unconditional (F6), and the previously-absent regression tests landed (F4). The non-aux path is provably unchanged (fn.MapOptionZ over None yields zero), so inputs without a resolution blob are unaffected.
Two concerns remain, both minor. The singleton-vs-set consistency (F2) is now defined by the new AuxSweeper contract, but that contract is enforced by docstring alone — a pre-existing out-of-tree implementation that predates it can silently violate additivity and desync the per-input filter credit from the per-set construction total. The per-input aux call inside the every-round loop (F3) is still O(n) per round with no batching, timeout, or caching. One genuinely-new minor: the freshly-added clamp/saturation branches have no direct test.
Nothing here rises to blocker or major on the current head.
Findings: 🔴 0 Blocker · 🟠 0 Major · 🟡 3 Minor · 🔵 0 Nit
Status of prior findings
- F1 addressed: Fixed. The
ExtraBudgetForInputserror branch now logs and setsextraBudget = 0, falling through to gate on the input's own budget rather thancontinue-dropping it — the stranding-on-error mode is gone. - F4 addressed: Fixed.
TestBudgetAggregatorFilterInputsAuxBudgetnow exercises the aux-rescue, aux-error-with-sufficient-own-budget, and aux-error-below-min-fee branches with a mockedAuxSweeper. - F5 addressed: Fixed.
extraBudgetis clamped to>= 0and folded into the own budget with a saturating add (overflow clamped tomath.MaxInt64), so the guard now only ever relaxes the filter relative to the input's own budget. - F6 addressed: Fixed. The
RequiredTxOutstub is now registered unconditionally with.Maybe(), so a regression that let the drop case fall through to the dust check surfaces as a clean assertion failure rather than an unstubbed-mock panic.
| // avoid. | ||
| extraBudget, err := fn.MapOptionZ( | ||
| b.auxSweeper, | ||
| func(aux AuxSweeper) fn.Result[btcutil.Amount] { |
There was a problem hiding this comment.
(wontfix) The two structural fixes the bot suggests are both heavier than the finding. A dedicated ExtraBudgetForInput (singular) on AuxSweeper would break the out-of-tree taproot-assets implementation until updated. A runtime cross-check (sum of singleton calls vs. the per-set total) doubles aux calls on a hot path that already runs every sweep round, for a strictly defense-in-depth check against an implementation we control. The contract is now explicit in the interface docstring, which is the appropriate level of enforcement for a minor, partial finding flagged as "reachability can't be confirmed from the diff."
|
/gateway dismiss F3 as per the comment: #10897 (comment) |
|
🚫 Dismissed F3 (minor) by @saubyk — as per the comment: #10897 (comment) Open findings on this PR: 🟠 F2 (major) · 🟡 F7 (minor) |
jtobin
force-pushed
the
htlc-force-close-sweeper
branch
2 times, most recently
from
31f4534 to
22a5862
Compare
| // avoid. | ||
| extraBudget, err := fn.MapOptionZ( | ||
| b.auxSweeper, | ||
| func(aux AuxSweeper) fn.Result[btcutil.Amount] { |
There was a problem hiding this comment.
(wontfix) The two structural fixes the bot suggests are both heavier than the finding. A dedicated ExtraBudgetForInput (singular) on AuxSweeper would break the out-of-tree taproot-assets implementation until updated. A runtime cross-check (sum of singleton calls vs. the per-set total) doubles aux calls on a hot path that already runs every sweep round, for a strictly defense-in-depth check against an implementation we control. The contract is now explicit in the interface docstring, which is the appropriate level of enforcement for a minor, partial finding flagged as "reachability can't be confirmed from the diff."
|
/gateway dismiss F2 per #10897 (comment) |
|
🚫 Dismissed F2 (major) by @saubyk — per #10897 (comment) Open findings on this PR: 🟡 F7 (minor) |
|
/gateway re-review |
|
✅ Re-review posted: #10897 (review) Prior findings: 5 addressed, 0 still unresolved, 0 withdrawn. 🔁 Need another re-review after pushing changes? Reply with |
There was a problem hiding this comment.
This revision resolves the remaining open items. The aux-error path now falls back to a zero contribution instead of dropping the input (F1), the negative-clamp and saturating-add guards are in place (F5), the RequiredTxOut stub is registered unconditionally (F6), and the test now pins both defensive branches with dedicated negative-clamp and overflow-saturation cases (F7). No prior finding remains unresolved.
Two minor observations remain on the newly-added defensive block, both grounded in the current diff and neither blocking: an asymmetry in the overflow guard (it clamps extraBudget but not the input's own budget), and per-input error logging that can amplify a single aux fault. The core fix — folding the aux contribution into the per-input filter so small custom-channel outputs aren't permanently stranded — is sound and well-tested.
Findings: 🔴 0 Blocker · 🟠 0 Major · 🟡 2 Minor · 🔵 0 Nit
Status of prior findings
- F1 addressed: Fixed in
sweep/aggregator.go:268— the aux-error branch now logs and setsextraBudget = 0, falling through to gate on the input's own budget rather thancontinue-ing the input out of the round. - F4 addressed: Fixed in
sweep/aggregator_test.go—TestBudgetAggregatorFilterInputsAuxBudgetnow covers the aux-rescue and aux-error paths with a mockedAuxSweeper, locking in theextraBudgetterm against future refactors. - F5 addressed: Fixed in
sweep/aggregator.go:285—extraBudgetis clamped to>= 0and added with a saturating add (extraBudget > math.MaxInt64-budget→MaxInt64), so an out-of-tree aux returning a negative or overflowing value can no longer tighten the filter or wrap negative. - F6 addressed: Fixed in
sweep/aggregator_test.go—mockInput.On("RequiredTxOut").Return(nil).Maybe()is now registered unconditionally, so a regression letting the drop case reach the dust check surfaces as a clean assertion failure rather than an unstubbed-mock panic. - F7 addressed: Fixed in
sweep/aggregator_test.go— the test table now includes "negative aux clamps to zero" (fn.Ok(-shortfall), expect kept) and "overflow saturates instead of wrapping" (near-MaxInt64own budget plus large positive extra, expect kept), pinning the two defensive branches added for F5.
| } | ||
|
|
||
| // Defensively clamp and saturate against a misbehaving aux | ||
| // implementation. The contract requires a non-negative, |
There was a problem hiding this comment.
(wontfix) The current state seems preferable; they're not O(n) identical entries, but an entry for each pending input, which seems right.
| } | ||
|
|
||
| // Defensively clamp and saturate against a misbehaving aux | ||
| // implementation. The contract requires a non-negative, |
There was a problem hiding this comment.
(wontfix) The current state seems preferable; they're not O(n) identical entries, but an entry for each pending input, which seems right.
The BudgetAggregator filters out inputs whose budget cannot cover the min relay fee or their requested starting fee rate. For inputs that carry a resolution blob (custom channel outputs), the aux sweeper contributes a sizable extra budget to any input set they join, but the filter only considered the input's own budget, which for asset outputs is tiny (their value is carried off-chain). The filter is mostly harmless with default parameters, but the starting fee rate of an input is ratcheted whenever a sweep attempt fails, including failures that have nothing to do with fees: e.g. when a concurrent sweep transaction spends the wallet UTXO that was backing this input's set (the sweeper currently doesn't lease selected wallet UTXOs, so concurrent input sets can pick the same one). One such collision is enough to push the required starting fee above a small asset input's own budget, after which the input is filtered out of every future input set and the sweep is silently stranded forever. Account for the aux extra budget in the filter, mirroring how the budget input set itself accounts for it when deciding whether wallet inputs are needed. Inputs without a resolution blob (the only kind that exists without an aux sweeper) are unaffected.
jtobin
force-pushed
the
htlc-force-close-sweeper
branch
from
22a5862 to
a86b85f
Compare
|
/gateway dismiss F9 per #10897 (comment) |
|
🚫 Dismissed F9 (minor) by @saubyk — per #10897 (comment) Open findings on this PR: 🟡 F8 (minor) |
|
/gateway re-review |
|
✅ Re-review posted: #10897 (review) Prior findings: 6 addressed, 0 still unresolved, 0 withdrawn. 🔁 Need another re-review after pushing changes? Reply with |
There was a problem hiding this comment.
This PR has converged. The original accounting fix — folding the aux sweeper's per-input extra budget into filterInputs so custom-channel-output inputs aren't permanently stranded — is sound, and every previously-raised concern is now either addressed in the code or dismissed by the maintainer. The last open item, the unguarded math.MaxInt64-budget headroom subtraction on a potentially-negative own budget (F8), is now closed by clamping budget to non-negative before the saturating add, with a dedicated test case pinning the branch.
The specialists' re-review surfaced no concern that isn't a restatement of a dismissed finding (F2 set-granularity/additivity, F3 per-round cost) or of the now-resolved defense-in-depth clamps (F5/F8). There are no genuinely-new findings.
Status of prior findings
- F1 addressed: Fixed in
sweep/aggregator.go— the aux-lookup error branch now logs and setsextraBudget = 0, falling through to gate the input on its own budget rather thancontinue-dropping it. The "silently stranded on aux error" mode is closed. - F4 addressed: Fixed in
sweep/aggregator_test.go—TestBudgetAggregatorFilterInputsAuxBudgetadds the aux-rescue case, the aux-error-keeps-sufficient case, and the aux-error-drops-below-min-fee case, locking in the fix. - F5 addressed: Fixed in
sweep/aggregator.go—extraBudgetis clamped to>= 0and the add is saturating (extraBudget > math.MaxInt64-budget→MaxInt64), so the fold-in can only relax the filter relative to the input's own budget. - F6 addressed: Fixed in
sweep/aggregator_test.go— theRequiredTxOutstub is now registered unconditionally with.Maybe(), so a regression letting the drop case fall through surfaces as a clean assertion failure rather than an unstubbed-mock panic. - F7 addressed: Fixed in
sweep/aggregator_test.go— the"negative aux clamps to zero"and"overflow saturates instead of wrapping"cases pin the two defensive branches added for F5. - F8 addressed: Fixed in
sweep/aggregator.go—budget := pi.params.Budget; if budget < 0 { budget = 0 }now runs before themath.MaxInt64-budgetheadroom check, so a negative own budget can no longer overflow the subtraction and falsely promote the input toMaxInt64. The new"negative own budget clamps to zero"test case pins the branch, and the comment now distinguishes the two failure modes.
|
/gateway approve |
There was a problem hiding this comment.
✅ Approved — all prior findings addressed.
Findings recap
Findings:
F1(major) — addressed: The newextraBudgeterror branch doescontinue, dropping the input fromfilteredInputswheneveraux.ExtraBudgetForInputsreturns an …F2(major) — partially_addressed:ExtraBudgetForInputsis invoked here per input over a singleton slice ([]input.Input{pi.Input}), and the full result is added to each in…F3(minor) — unresolved:aux.ExtraBudgetForInputsis called inside the per-input loop, once per pending input, every sweep round, whereas the set-construction path…F4(minor) — addressed: The diff adds three behavioral branches — the errorcontinueand the two guards now comparingpi.params.Budget + extraBudget— with no a…F5(minor) — addressed:budget := pi.params.Budget + extraBudgetadds an externally-sourcedbtcutil.Amount(signed int64) fromAuxSweeper.ExtraBudgetForInputs…F6(minor) — addressed: InTestBudgetAggregatorFilterInputsAuxBudgetthemockInput.On("RequiredTxOut").Return(nil)stub is registered only whentc.expectKept…F7(minor) — addressed: The new test covers the aux-rescue and aux-error paths but adds no case for the two newly-introduced defensive branches: the negative clamp …F8(minor) — addressed: The defensive block clampsextraBudget < 0to zero and usesif extraBudget > math.MaxInt64-budgetas the overflow guard, but never estab…F9(minor) — unresolved: The aux-error branch callslog.Errorf("Unable to fetch extra budget for input=%v, falling back to own budget: %v", op, err)inside the per…
Dismissed:
F3by @saubyk — as per the comment: #10897 (comment)F2by @saubyk — per #10897 (comment)F9by @saubyk — per #10897 (comment)
Approved by @saubyk via /gateway approve. Last reviewed at a86b85f. Skill v0.3.0, model claude-opus-4-8.
3 participants
(TLDR, fixes an accounting error that can lead to sweep failures after aux channel force closes. Related to lightninglabs/taproot-assets#1888 and lightninglabs/taproot-assets#1976).
The BudgetAggregator filters out inputs whose budget cannot cover the min relay fee or their requested starting fee rate. For inputs that carry a resolution blob (custom channel outputs), the aux sweeper contributes a sizable extra budget to any input set they join, but the filter only considered the input's own budget, which for asset outputs is tiny.
The filter is mostly harmless with default parameters, but the starting fee rate of an input is ratcheted whenever a sweep attempt fails, including failures that have nothing to do with fees, e.g. when a concurrent sweep transaction spends the wallet UTXO that was backing this input's set (#10816 contains a separate, independent fix for the "always ratchet on every error" issue, which I need to tease out from the flawed leasing approach introduced there). One such collision is enough to push the required starting fee above a small asset input's own budget, after which the input is filtered out of every future input set and the sweep is silently stranded forever.
This change accounts for the aux extra budget in the filter, mirroring how the budget input set itself accounts for it when deciding whether wallet inputs are needed. Inputs without a resolution blob (the only kind that exists without an aux sweeper) are unaffected.