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
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
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
| log.Errorf("Unable to fetch extra budget for "+ | ||
| "input=%v: %v", op, err) | ||
|
|
||
| continue |
There was a problem hiding this comment.
🟠 Major F1: Aux-budget lookup error silently drops the input
The new extraBudget error branch does continue, dropping the input from filteredInputs whenever aux.ExtraBudgetForInputs returns an error. Before this change the only exclusions were structural/economic (unknown size, budget below fee, dust) — pi.params.Budget is a plain field read that cannot fail. filterInputs runs every sweep round over all pending inputs, so a transient or persistent aux-sweeper error now evicts an otherwise-sweepable output from every future round with no economic justification — the exact "silently stranded forever" mode this PR fixes, relocated to the error path. The conservative, pre-existing behavior is recoverable by treating a lookup failure as zero extra budget (extraBudget = 0, fall through) so the input is still filtered on its own budget rather than dropped outright. Confirm the drop-on-error is intentional rather than a fallback to zero.
|
|
||
| continue | ||
| } | ||
| budget := pi.params.Budget + extraBudget |
There was a problem hiding this comment.
🟠 Major F2: Per-input singleton extra budget vs per-set accounting
ExtraBudgetForInputs is invoked here per input over a singleton slice ([]input.Input{pi.Input}), and the full result is added to each input's own budget at budget := pi.params.Budget + extraBudget. The PR body states this mirrors how the budget input set accounts for it, but NewBudgetInputSet computes the extra budget once for the whole input slice — a single pool shared across the set. If the aux sweeper attributes budget at set granularity, then when N small asset inputs join the same set the filter credits that shared pool N times, potentially admitting inputs whose set cannot pay aggregate fees once the set-level accounting counts the extra budget only once. This may be a non-issue if ExtraBudgetForInputs([singleton]) is defined to return that input's individual share rather than a set total — I cannot determine which from the diff. Confirm the singleton-vs-set semantics so the per-input filter credit and the per-set set construction stay consistent.
| extraBudget, err := fn.MapOptionZ( | ||
| b.auxSweeper, | ||
| func(aux AuxSweeper) fn.Result[btcutil.Amount] { | ||
| return aux.ExtraBudgetForInputs( |
There was a problem hiding this comment.
🟡 Minor F3: Extra budget fetched once per input each sweep round
aux.ExtraBudgetForInputs is called inside the per-input loop, once per pending input, every sweep round, whereas the set-construction path computes it once per set. If the aux implementation's per-call cost is non-trivial this is O(n) aux calls per round, scaling with the number of pending inputs. Consider batching into a single set-level call to bound the work and match the once-per-set convention used elsewhere.
| // for the extra budget here we'd filter such inputs out | ||
| // permanently, even though their input set could comfortably | ||
| // pay its fees. | ||
| extraBudget, err := fn.MapOptionZ( |
There was a problem hiding this comment.
🟡 Minor F4: No test for the new extra-budget filter branch
The diff adds three behavioral branches — the error continue and the two guards now comparing pi.params.Budget + extraBudget — with no accompanying test. A regression test with a mocked AuxSweeper returning a non-trivial extra budget for a low-own-budget input (the exact case being fixed), plus a case for the error path, would lock in the fix; without it a future refactor dropping the extraBudget term silently reintroduces the stranding bug.
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
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.