Repository navigation
Conversation
Unsigned transaction construction and fee lookup can fail before signing. Treating those failures as signing errors stops the shared batcher and loopd. A transient wallet failure can trigger this. Log these failures and retry only the affected batch. Leave signing failures fatal, and verify unrelated batches still publish.
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
|
/gateway review |
![lightninglabs-gateway[bot]](https://avatars.githubusercontent.com/in/3635431?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gateway review — 5 findings
🔴 0 Blocker · 🟠 2 Major · 🟡 3 Minor · 🔵 0 Nit
Summary
This PR moves the error boundary so that a failed fee lookup or a failed unsigned-tx build is logged and retried, instead of stopping the whole batcher and loopd. The sentinel-error approach is clean. Keeping signing failures fatal is reasonable.
It falls short in two places. First, the boundary is per batch, not per swap. A sweep that can never be built now stays in its batch indefinitely and takes every sweep sharing that batch with it. Before this change it stopped everything immediately. Second, only MinRelayFee and constructUnsignedTx are wrapped. Other failures that happen before signing, some of them transient calls to external services, still reach the fatal default branch. The presigned path changes also have no test.
Lower-severity findings (3)
🟡 F3 (Minor) — Fail flag is armed after AddSweep, so the test depends on timing · sweepbatcher/construction_error_test.go:179
failingWallet.fail.Store(true) runs after AddSweep returns. The one-shot failure then goes to whichever MinRelayFee call comes first, which may not be the one in publishMixedBatch. Either arm the flag before AddSweep, or make the wrapper fail only on calls from the publish path.
🟡 F4 (Minor) — Presigned construct-error path has no test · sweepbatcher/presigned.go:477
publishPresigned construction failures (lines 446 and 477) used to be fatal and are now retried every block, but neither test case runs through this function. Both go through publishMixedBatch, as the "failed to get min relay fee" message shows. Add a presigned-mode case that checks the batch stays pending, keeps logging "construct error", and does not stop the batcher.
🟡 F5 (Minor) — Failed construct retries may keep raising the fee rate · sweepbatcher/sweep_batch.go:1263
updateRbfRate runs before every publish attempt at line 1229, and the new construct-error path returns nil without publishing anything. If updateRbfRate raises rbfCache.FeeRate on each call (its body is not in context), a fee or dust failure gets worse on every retry instead of having a chance to recover. Before this PR that path could not be reached.
Reviewed at medium effort.
Bot commands
/gateway re-review— re-run after pushing changes (maintainers)/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
| return nil | ||
| } else { | ||
|
|
||
| case errors.Is(err, errBatchNotConstructed): |
There was a problem hiding this comment.
🟠 F1 (Major) — Unconstructable sweep stalls other sweeps in its batch · sweepbatcher/sweep_batch.go:1262
A sweep that can never be built now stays in its batch indefinitely, and every other sweep in that batch is stuck with it. Previously the node stopped outright. Now some sweeps can miss their deadlines while the node stays up. For loop-out sweeps, missing the HTLC CLTV deadline lets the server take the timeout path, so the cost is a possible fund loss on honest sweeps that share the batch.
Why this matters
constructUnsignedTx checks all sweeps in the batch together. One problem swap fails the whole batch: per-swap inputs <= change, an output below dust, or fee+sumChange+dustLimit > batchAmt. After this change that failure is wrapped in errBatchNotConstructed, publish returns nil, and the batch stays Open. It fails the same way on every block. Nothing in the diff removes the bad sweep, splits the batch, or signals the swap's SpendNotifier. Before this change the error was fatal, so a failing batch never lived long enough to take in more sweeps.
The new test avoids this case on purpose. Both sweeps set IsExternalAddr: true and use different timeouts "to force separate batches" (construction_error_test.go:115, 190-191). Batch admission and eviction are not in the loaded context, so I can't confirm whether something already keeps healthy sweeps out of a batch that has failed to build. If nothing does, either stop the batch from accepting sweeps after errBatchNotConstructed, or quarantine the failing sweep and re-batch the rest. Add a test where a healthy, non-external sweep joins the same batch as the 200-sat sweep.
| logPublishError("construct error", err) | ||
| return nil | ||
|
|
||
| default: |
There was a problem hiding this comment.
🟠 F2 (Major) — Other pre-signing failures still stop the batcher · sweepbatcher/sweep_batch.go:1266
Several failures that happen before signing are still not wrapped with errBatchNotConstructed. They fall into the default branch, get logged as "signing error", and are returned from batch.Run at line 1017. That stops the shared batcher and loopd, which is the failure mode this PR sets out to remove. A transient wallet or helper error on these paths is the same kind of failure as the MinRelayFee error the PR now tolerates.
Why this matters
The unwrapped paths are:
getPresignedSweepsDestAddrat presigned.go:467-469, which calls the externalpresignedHelper.DestPkScript.getBatchDestAddrat line 1576. Its body is not in context; if it calls the wallet, this is a real gap.createPsbtat line 1617.updateRbfRateat line 1231.
The sentinel's comment says it "marks failures before the transaction is signed", and the release note promises the batch "retries without stopping loopd". Either wrap these errors with errBatchNotConstructed the same way as presigned.go:446/477, or narrow the comment and release note to cover only fee lookup and tx construction.
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
Keep new sweeps out of a batch after construction fails, including batches restored before their first retry. Treat remaining preparation failures as retryable while keeping signing failures fatal. Restore the previous fee state when no transaction is constructed and cover regular and presigned retries with deterministic tests.
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review
No findings.
Status of prior findings
- F1 addressed: Fixed in sweepbatcher/sweep_batch.go:516-518 and :821-823. A batch whose construction failed has
admissionPausedset at :1251 and :1284. While it is set,checkSweepToAddrejects new sweeps andmatchesSweepModereports no match. A batch restored from the DB starts paused (:440). The new test adds a healthy, non-external sweep with the same timeout (900) after the failure. It asserts that the sweep lands in a second batch, which then publishes (construction_error_test.go:252-261). Sweeps already in the batch stay with it, but adding a healthy input to a regular batch doesn't make construction fail on dust or fee grounds. That leaves no realistic way for this to poison other sweeps. - F2 addressed: Fixed: every path flagged last time is now wrapped with
errBatchNotConstructed. That coversgetPresignedSweepsDestAddr(presigned.go:468),getBatchDestAddr(sweep_batch.go:1604),createPsbt(:1647) and theEstimateFeeRatecall inupdateRbfRate(:2050). TheupdateRbfRateerror is handled locally at :1250-1254. - F3 addressed: Fixed:
WithInitialDelaynow blocks onallowPublish, and the failure flag is armed beforeclose(allowPublish)(construction_error_test.go:210-213). The singleMinRelayFeefailure is therefore used by the first publish attempt, which the test confirms withwantError. - F4 addressed: Fixed: the new "presigned wallet fee lookup" case (construction_error_test.go:62-68) goes through
publishPresigned. It checks for the "failed to get minRelayFeeRate" construct error twice: on the first attempt and on the retry at height 601, without the batcher stopping. - F5 addressed: Fixed:
publishsavespreviousRbfCachebeforeupdateRbfRate(sweep_batch.go:1246). On a construction error it restores and persists that copy (:1285-1289). The fee-bumping test checks thatrbfCache.FeeRateis unchanged across two failed attempts (construction_error_test.go:241-246).
Reviewed at medium effort.
Bot commands
/gateway re-review— re-run after pushing changes (maintainers)/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
Summary
sweep batch in regular and presigned publishing. Signing and persistence
failures remain fatal.
sweeps can enter another batch. Restored batches remain paused until they
construct a transaction successfully.
retries do not keep raising the fee rate without a published transaction.
with the same timeout continuing in another batch.
Scope
The shared batcher previously returned these preparation errors to
loopd,stopping unrelated sweeps. A permanently unconstructable batch remains
pending and continues to log warnings for operator attention. This does not
claim to reproduce the reported attacker-controlled low-value HTLC path
through normal swap admission and restart.
Validation
make fmtgo test ./sweepbatcher/... -count=1golangci-lint run --new-from-rev=origin/master ./sweepbatcher/...