sweep: don't ratchet starting fee rate on non-fee failures#10898
sweep: don't ratchet starting fee rate on non-fee failures#10898jtobin wants to merge 2 commits into
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 issue where the sweeper would incorrectly increase the starting fee rate for sweep attempts that failed due to resource constraints rather than fee-related issues. By distinguishing between fee-related failures and resource-related failures, the system now avoids ratcheting the fee rate when it cannot resolve the underlying problem, ensuring that inputs are not unnecessarily stranded. 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 modifies the sweeper's fee bumping logic to avoid ratcheting up the starting fee rate on resource-related failures (such as ErrNotEnoughInputs and ErrNotEnoughBudget). Instead, these failures preserve the existing starting fee rate to prevent inputs from being permanently stranded. Corresponding unit tests have been added and updated to verify this behavior. A review comment suggests correcting a grammatical error in a warning log message from 'Fail to' to 'Failed to'.
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.
| errors.Is(err, ErrNotEnoughInputs) { | ||
|
|
||
| // Calculate the next fee rate for the retry. | ||
| log.Warnf("Fail to fee bump tx %v: %v", oldTx.TxHash(), err) |
There was a problem hiding this comment.
The log message uses the incorrect verb tense "Fail to". It should be "Failed to" to be grammatically correct and consistent with other log messages in this file (e.g., "Failed to bump tx").
| log.Warnf("Fail to fee bump tx %v: %v", oldTx.TxHash(), err) | |
| log.Warnf("Failed to fee bump tx %v: %v", oldTx.TxHash(), err) |
github-actions
Bot
added
the
severity-critical
|
PR Severity: CRITICAL. All changed files are in sweep/* (fee_bumper.go and sweeper.go), which handles output sweeping, fund recovery, and fee bumping — a CRITICAL package. 2 non-test files, 86 non-test lines changed (well under bump thresholds). Test files excluded: fee_bumper_test.go, sweeper_test.go. To override, add a severity-override-{critical,high,medium,low} label. <!-- pr-severity-bot --> |
jtobin
force-pushed
the
sweep-fee-ratchet
branch
from
893025b to
b1da0b2
Compare
|
@jtobin, remember to re-request review from reviewers when ready |
|
/gateway review |
|
✅ Review posted: #10898 (review) 6 finding(s); 6 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 stops the sweeper from ratcheting an input's starting fee rate on resource failures (ErrNotEnoughInputs, ErrNotEnoughBudget), which previously could push the starting rate past an input's budget and silently strand the sweep. The intent is sound and the fix is well-targeted, and the new tests directly assert the no-ratchet behavior on both the initial and replacement paths.
The mechanism, however, is a magic value: a BumpResult with FeeRate == 0 now means "preserve the existing starting rate," consumed by markInputsPublishFailed. This is only correct if zero is produced exclusively by the two resource-failure branches and never by a legitimate fee computation — and the retained ErrMaxPosition branch and the "all other failures" branch in handleReplacementTxError both still attach calculateRetryFeeRate(r) to a TxFailed result. The correctness of the entire change rests on that disjointness, which the visible diff does not establish. On a CRITICAL fund-recovery path this needs to be confirmed (or the intent carried explicitly) before merge. Secondary concerns: the two sibling error handlers now classify the same error set with divergent code shapes, and a comment carried over from the old shared-case structure now misdescribes ErrMaxPosition.
Findings: 🔴 0 Blocker · 🟠 2 Major · 🟡 4 Minor · 🔵 0 Nit
| case errors.Is(err, ErrNotEnoughInputs), | ||
| errors.Is(err, ErrNotEnoughBudget): | ||
|
|
||
| result.Event = TxFailed |
There was a problem hiding this comment.
🟠 Major F2: Preserving the rate on budget failure may strand a deadline-sensitive sweep
For ErrNotEnoughBudget/ErrNotEnoughInputs the input now retries indefinitely at its original starting fee rate. For a budget-exhausted input that is the intended fix, but the same branch is reachable for inputs with a confirmation deadline (e.g. an outgoing-HTLC timeout sweep) whose budget is momentarily insufficient while the deadline still approaches. If deadline pressure escalates the fee only through the now-skipped ratchet path, such a sweep can sit at a low rate and miss its deadline — a fund-safety regression and a potential griefing lever for a counterparty that can keep a channel near its budget ceiling. I cannot tell from the diff whether a separate deadline-driven escalation (independent of this ratchet) keeps time-sensitive sweeps moving; the relevant fee-function/deadline code is not in the loaded context. Confirm that preserving the starting rate on a budget failure cannot strand a deadline-bound sweep before relying on this behavior; if it can, the resource-failure handling needs to distinguish deadline-bound inputs.
There was a problem hiding this comment.
Left as-is — false positive; ratcheting the starting rate can't rescue a deadline-bound sweep failing ErrNotEnoughBudget (budget is the cap by definition), and in-attempt fee escalation still runs via feeFunc.Increment() independent of this path.
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
jtobin
force-pushed
the
sweep-fee-ratchet
branch
from
b1da0b2 to
290d70e
Compare
|
/gateway re-review |
|
Sorry, |
|
/gateway re-review |
|
✅ Re-review posted: #10898 (review) Prior findings: 5 addressed, 0 still unresolved, 1 withdrawn. 🔁 Need another re-review after pushing changes? Reply with |
There was a problem hiding this comment.
This revision cleanly addresses all six prior findings: the feeRate == 0 overload is replaced by an explicit fn.Option, the predicate is deduplicated, the misleading comment is fixed, the log typos are corrected, and the disjointness between ErrMaxPosition (ratchets) and resource failures (preserves) is now locked in by a test. Prior F2 (deadline-bound stranding) is withdrawn — with the fuller context now available, per-attempt fee-function re-initialization from the deadline conf target keeps time-sensitive sweeps escalating independently of the preserved starting rate, so the concern does not apply.
One genuinely-new, minor concern: the fn.None-means-preserve default now applies to every TxFailed path that doesn't explicitly set a rate — not just the resource-failure case the PR describes.
This was the third reorganization of this fee-result classification in ~4 months (PRs #9447, #9627); the contract now looks settled, but it remains a churn hotspot worth coordinating with its prior author.
Findings: 🔴 0 Blocker · 🟠 0 Major · 🟡 1 Minor · 🔵 0 Nit
Status of prior findings
- F1 addressed: Fixed in
sweep/fee_bumper.go:274andsweep/sweeper.go:998— thefeeRate == 0overload is replaced byNextStartingFeeRate fn.Option[chainfee.SatPerKWeight];markInputsPublishFailednow branches onIsNone()(preserve) vsSome(ratchet), so "no fee signal" and "a computed rate of zero" are no longer conflated. - F2 withdrawn: On second look, this was incorrect. Preserving the starting fee rate on a resource failure cannot strand a deadline-bound sweep: each retry re-initializes the fee function via
NewLinearFeeFunctionusingcalcCurrentConfTarget(currentHeight, DeadlineHeight), so deadline pressure escalates the rate through the estimator independently of the (now-preserved) starting rate, andcreateRBFCompliantTxstill increments within an attempt. ForErrNotEnoughBudgetspecifically, the budget is the cap by definition, so a higher starting rate could not have rescued the sweep regardless. Retracting. - F3 addressed: Fixed —
TestHandleInitialTxErrorRatchetsOnErrMaxPosition(sweep/fee_bumper_test.go) assertsErrMaxPositionstill callsIncrementand carriesfn.Some(retryRate), locking in the disjointness from the resource-failure branch. - F4 addressed: Fixed — the
ErrMaxPositioncase comment now describes linear-fee-function position-window exhaustion instead of budget exhaustion (sweep/fee_bumper.go:1141). - F5 addressed: Fixed —
isResourceFailure(err error) boolis extracted and referenced from bothhandleInitialTxErrorandhandleReplacementTxError, so a future third resource error can't be added to one site and missed at the other. - F6 addressed: Fixed —
"Fail to fee bump tx"→"Failed to fee bump tx"at both call sites inhandleReplacementTxError, matching the sibling"Failed to bump tx"statements.
jtobin
force-pushed
the
sweep-fee-ratchet
branch
from
290d70e to
fc3095d
Compare
|
@saubyk One more should do it. 🤞 |
|
/gateway re-review |
|
✅ Re-review posted: #10898 (review) Prior findings: 6 addressed, 0 still unresolved, 1 withdrawn. 🔁 Need another re-review after pushing changes? Reply with |
There was a problem hiding this comment.
This re-review covers the small diff landed since the last review (290d70e…fc3095d), which is the documentation tightening for the previously-open finding F7. That widening — fn.None now preserving the starting rate across ErrTxNoOutput, ErrZeroFeeRateDelta, and the broadcast-error fallback, not just the resource failures — is now explicitly documented on BumpResult.NextStartingFeeRate, on markInputsPublishFailed, and in the inline comment, so F7 is resolved. All other prior findings remain addressed or withdrawn.
One genuinely-new minor surfaced: in handleReplacementTxError, when calculateRetryFeeRate fails and the result is downgraded to TxFatal, the fee-calculation error is silently dropped (the result keeps the original bump error and ferr is never logged), diverging from the two sibling paths that attribute the fatal transition to the calc error. Functionally the input is correctly made fatal, so this is an error-attribution/observability nit rather than a behavioral defect.
Findings: 🔴 0 Blocker · 🟠 0 Major · 🟡 1 Minor · 🔵 0 Nit
Status of prior findings
- F1 addressed: Resolved. The
FeeRate == 0overload was replaced byBumpResult.NextStartingFeeRate fn.Option[chainfee.SatPerKWeight](sweep/fee_bumper.go:281), withmarkInputsPublishFailedbranching on the Option (sweep/sweeper.go:1007) — exactly the explicit-intent fix the original finding recommended. "No fee signal" (None) and "a computed rate of zero" (Some(0)) are now distinguishable. - F2 withdrawn: Remains withdrawn. Ratcheting the starting rate cannot rescue a budget-capped, deadline-bound sweep (the budget is the hard cap on
MaxFeeRateAllowed), and within-attempt escalation via the fee function'sIncrement()/IncreaseFeeRateis independent of the preservedStartingFeeRate. Preserving on a resource failure introduces no new deadline-miss path. - F3 addressed: Resolved.
TestHandleInitialTxErrorRatchetsOnErrMaxPosition(sweep/fee_bumper_test.go) now assertsErrMaxPositionstill callsIncrementand carriesfn.Some(retryRate), locking in the disjointness from the no-ratchet resource branch. - F4 addressed: Resolved. The
ErrMaxPositioncase comment was reworded to describe linear-fee-function position-window exhaustion rather than budget exhaustion (sweep/fee_bumper.go:1131). - F5 addressed: Resolved.
isResourceFailure(err error) boolwas extracted (sweep/fee_bumper.go:1115) and referenced from bothhandleInitialTxErrorandhandleReplacementTxError, so a future third resource error can't be added to one site and missed at the other. - F6 addressed: Resolved. "Fail to fee bump tx" → "Failed to fee bump tx" at both
handleReplacementTxErrorcall sites, matching the sibling "Failed to bump tx" log. - F7 addressed: Resolved. The
fn.None-means-preserve scope widening (coveringErrTxNoOutput,ErrZeroFeeRateDelta, and the broadcast-error fallback, not onlyisResourceFailure) is now documented onBumpResult.NextStartingFeeRate(sweep/fee_bumper.go:277), onmarkInputsPublishFailed(sweep/sweeper.go:967), and in the inline comment (sweep/sweeper.go:1008). The preserve semantics are explicitly stated as intended for these non-resource paths.
jtobin
force-pushed
the
sweep-fee-ratchet
branch
from
fc3095d to
8ec5b63
Compare
github-actions
Bot
added
severity-critical
When a sweep fails to publish, the sweeper computes a higher retry fee rate and stores it as the input's new starting rate. That's the right move for fee-related failures, where the broadcast was rejected for not paying enough, but the same machinery also fires on resource failures (no wallet UTXO available, the input set's budget exceeded), which carry no fee-rate signal. Repeatedly ratcheting the rate on these can push it past an input's intrinsic budget, after which the aggregator silently skips the input and the sweep is stranded forever. Distinguish fee-rate-bearing failures from resource ones at the point where the bump result is produced, and skip the rate ratchet for the latter: the input's existing starting fee rate is preserved across non-fee failures, so the next retry starts at the rate the original sweep request specified.
jtobin
force-pushed
the
sweep-fee-ratchet
branch
from
8ec5b63 to
2fb8325
Compare
|
/gateway re-review |
|
✅ Re-review posted: #10898 (review) Prior findings: 7 addressed, 0 still unresolved, 1 withdrawn. 🔁 Need another re-review after pushing changes? Reply with |
There was a problem hiding this comment.
This re-review clears the last open item: F8 (the swallowed ferr attribution in handleReplacementTxError) is now addressed — the fatal branch sets bumpResult.Err = ferr, matching handleInitialTxError and createUnknownSpentBumpResult. With that, all eight prior findings are resolved (F2 remains withdrawn). The core NextStartingFeeRate fn.Option contract — fn.None preserves, fn.Some ratchets — is consistently applied across producer sites and the markInputsPublishFailed consumer, and is well covered by the new tests.
One genuinely-new, minor observation: the calculateRetryFeeRate → fatal-on-error → fn.Some on-success pattern is now triplicated. No new defects; the specialists' remaining concerns all hinged on code outside this diff (calculateRetryFeeRate's zero/monotonicity guarantees, sibling PR #10897) and are not actionable from the visible change.
Findings: 🔴 0 Blocker · 🟠 0 Major · 🟡 1 Minor · 🔵 0 Nit
Status of prior findings
- F1 addressed: Fixed —
BumpResult.NextStartingFeeRate fn.Option[chainfee.SatPerKWeight]now carries intent explicitly (fn.None= preserve,fn.Some= ratchet), andmarkInputsPublishFailedbranches onIsNone()rather than overloadingfeeRate == 0. This is exactly the robust fix the original finding recommended. - F2 withdrawn: Remains withdrawn. Preserving (not raising)
StartingFeeRateonErrNotEnoughBudgetcannot strand a deadline-bound sweep: the budget is the binding cap, a higher seed rate can only push the input above its own budget-derived max (the stranding mechanism this PR removes), and in-attempt escalation via the fee function'sIncrementis unaffected. - F3 addressed: Fixed —
TestHandleInitialTxErrorRatchetsOnErrMaxPositionassertsErrMaxPositionstill callsIncrementand carriesfn.Some(retryRate), locking in the disjointness from the resource-failure branch. - F4 addressed: Fixed in
sweep/fee_bumper.go— theErrMaxPositioncase comment now describes linear-fee-function position-window exhaustion rather than budget exhaustion. - F5 addressed: Fixed —
isResourceFailure(err error) boolis extracted and referenced from bothhandleInitialTxErrorandhandleReplacementTxError, keeping the resource-error set in one place. - F6 addressed: Fixed — both relocated warnings now read "Failed to fee bump tx", matching the sibling
log.Errorf("Failed to bump tx ..."). - F7 addressed: Fixed — the
NextStartingFeeRatefield doc and themarkInputsPublishFaileddoc now both enumerate thatfn.NonecoversisResourceFailureplus the non-fee initial errorsErrTxNoOutputandErrZeroFeeRateDelta, documenting the widened preserve scope. - F8 addressed: Fixed in
sweep/fee_bumper.go— theferr != nilbranch ofhandleReplacementTxErrornow setsbumpResult.Err = ferr, so an operator paged on a permanently-removed input sees the actual fee-calculation failure rather than the original bump error, matchingcreateUnknownSpentBumpResultand theErrMaxPositionarm ofhandleInitialTxError.
|
|
||
| log.Warnf("Fail to fee bump tx %v: %v", oldTx.TxHash(), err) | ||
| return result | ||
| feeRate, ferr := t.calculateRetryFeeRate(r) |
There was a problem hiding this comment.
(wontfix) I'll avoid addressing this unless there's demand to do so. The repetitive style seems common.
|
|
||
| log.Warnf("Fail to fee bump tx %v: %v", oldTx.TxHash(), err) | ||
| return result | ||
| feeRate, ferr := t.calculateRetryFeeRate(r) |
There was a problem hiding this comment.
(wontfix) I'll avoid addressing this unless there's demand to do so. The repetitive style seems common.
|
/gateway dismiss F9 per #10898 (comment) |
|
🚫 Dismissed F9 (minor) by @saubyk — per #10898 (comment) All findings on this PR are addressed or dismissed. |
|
/gateway approve |
There was a problem hiding this comment.
✅ Approved — all prior findings addressed.
Findings recap
Findings:
F1(major) — addressed:markInputsPublishFailednow treatsfeeRate == 0as "no fee dimension — preserve the input's existingStartingFeeRate" andcontinues …F2(major) — withdrawn: ForErrNotEnoughBudget/ErrNotEnoughInputsthe input now retries indefinitely at its original starting fee rate. For a budget-exhausted i…F3(minor) — addressed: The refactor splits the former fallthrough soErrMaxPositionkeeps callingcalculateRetryFeeRatewhile the resource errors no longer do.…F4(minor) — addressed: The rewritten comment oncase errors.Is(err, ErrMaxPosition)opens "When the error is due to budget being used up" — wording carried over …F5(minor) — addressed:errors.Is(err, ErrNotEnoughInputs) || errors.Is(err, ErrNotEnoughBudget), plus a near-identical multi-line justification, now appears in b…F6(minor) — addressed: The relocated warning logs"Fail to fee bump tx %v: %v", inconsistent with the siblinglog.Errorf("Failed to bump tx %v: %v", ...)a few…F7(minor) — addressed:NextStartingFeeRatedefaults tofn.None, andmarkInputsPublishFailedtreatsNoneas "preserve the existing starting fee rate." This …F8(minor) — addressed: InhandleReplacementTxError, whencalculateRetryFeeRatereturnsferr != nilthe result is downgraded toTxFatalbutbumpResult.Err…F9(minor) — unresolved: The same retry-fee-rate sequence —feeRate, err := t.calculateRetryFeeRate(r); on error setEvent=TxFatal/Err, otherwise `NextStarti…
Dismissed:
F9by @saubyk — per #10898 (comment)
Approved by @saubyk via /gateway approve. Last reviewed at 2fb8325. Skill v0.3.0, model claude-opus-4-8.
3 participants
Supplementary to #10897 (and replaces #10816).
The sweeper responds to a failed sweep attempt by ratcheting the input's fee rate for the next one. For fee-related failures this is the right response, but the same machinery also fires on resource failures, e.g. ErrNotEnoughInputs or ErrNotEnoughBudget. These failures carry no fee rate signal, so ratcheting the fee rate can't resolve them.
Each spurious ratchet raises the bar the input's budget must clear, and once the starting rate exceeds an input's budget, the aggregator filters it out of every future input set and the sweep is silently stranded. This PR skips the ratchet for failures with no fee dimension, preserving the input's existing starting fee rate across retries.
The TxUnknownSpend path also attaches a retry fee rate. It's deliberately left untouched here, since an unknown spend can reflect a genuine fee race with a counterparty; with #10897, that residual ratchet can no longer strand small-budget inputs.