AddSweep now masks cancellation races during its caller-side setup, but
a request can still be accepted by the batcher run loop before the run
context cancellation is selected. If handleSweeps then performs a
context-sensitive store call while shutdown is in progress, a
backend-specific error such as driver.ErrBadConn can still bubble out
through Batcher.Run.
Prefer the run context's terminal error when startup, handleSweeps, or
asynchronous errChan failures happen after the run context has been
canceled. This keeps normal shutdown reporting consistent and avoids
surfacing backend driver errors from the accepted-request and async
error paths.
Log the original run-loop error before returning the context error so
normal shutdown remains debuggable without changing the returned error.
Add regression tests for both covered run-loop races. One lets AddSweep
successfully hand a sweep request to Run, then cancels the run context
from the second GetSweepStatus call while returning driver.ErrBadConn.
The other queues an errChan error from the event loop while canceling
the run context. Both tests assert that Run returns context.Canceled and
does not wrap the driver error.
Fixes a crash window where handleConf updated the batch row to confirmed but
failed before marking sweeps complete, so re-added sweeps spawned a duplicate
batch or kept retrying. Batch confirmation and sweep completion are now
persisted inside a single DB transaction ConfirmBatchWithSweeps, and handleConf
uses the helper to atomically store the batch and the set of confirmed sweeps.
Added TestSweepBatcherConfirmedBatchIncompleteSweeps that runs against the real
loopdb backend, injects a failure mid-transaction, and verifies the database
never ends up with confirmed=true batches paired with completed=false sweeps.
Started from CI failure of TestSweepBatcherHandleSweepRace/loopdb [1] that
surfaced ErrBatcherShuttingDown when the AddSweep loop caught a batch right as
it finished. Reproduced via:
go test ./sweepbatcher -run TestSweepBatcherHandleSweepRace/loopdb -cpu=12,4,8 -count=1
Discovered that handleSweeps treated ErrBatchShuttingDown from batch.addSweeps
as fatal, so the batcher exited even though the sweep was already confirmed.
Fixed by treating that error as "batch already done" so we fall through to the
persisted status/monitorSpend path, and added a regression test that
deterministically simulates the shutdown window.
New regression test added:
go test ./sweepbatcher -run TestSweepBatcherHandleBatchShutdown
[1] https://github.com/lightninglabs/loop/actions/runs/19282552307/job/55136597881?pr=1041
If a SignTx call is slow, the whole presign() function could timeout if all
the calls are done sequentially. In this commit each call is done in a separate
goroutine to reduce total latency.
We forgot to account for change outputs when checking the feerate of
signed transaction. The bug resulted in fee rate overestimation in the
log message.
Presigned sweeps that produce a change output misreported the on-chain fee.
The fee portion was derived from the total swept amount minus only the
first transaction output, so any change output was treated as additional fee.
Update getFeePortionForSweep to subtract the value of every tx output so
the fee portion reflects only the actual miner fee paid. Add regression
coverage that sweeps a presigned input with change and asserts the spend
and confirmation notifications report the corrected fee.
Re-ordered the cleanup in handleSweeps so completed batches are deleted only
after any re-add attempt, preventing a finished batch from being dropped before
it can absorb the re-add.
Added a fresh status/parent-batch lookup inside handleSweeps so the logic now
reacts to swaps that complete between AddSweep and the event-loop handler; fully
confirmed sweeps are routed straight to monitorSpendAndNotify instead of
spawning a duplicate batch.
Added regression test testSweepBatcherHandleSweepRace, which replays the
AddSweep/confirmation race and verifies that only the original batch remains and
the sweep keeps its parent assignment.
The Problem:
The test TestPresigned/dust_main_output was causing a deadlock. When the test
called batcher.AddSweep for the second time (with a valid sweep), the batcher
would create a new batch and attempt to register for a spend notification.
However, the test was not waiting for or consuming this notification
registration, leading to a blocked goroutine and a timeout.
The Fix:
I have added a line to the test to consume the spend notification registration
from the mock lnd's channel, similar to how other tests in the same file are
structured. This prevents the deadlock.
The problem was introduced in commit 54652dc641.
- ensurePresigned: use passed minRelayFeeRate instead of chainfee.FeePerKwFloor
- presign: use minRelayFeeRate for start and minRelayFee
- presign: make sure minRelayFeeRate is set
- add tests for presign to test this new behavior; make sure the number of
transactions is lower if minRelayFeeRate is higher
- update error message in constructUnsignedTx: use <, not <= (more accurate)
- use utils.DustLimitForPkScript instead of lnwallet.DustLimitForSize in tests
- in tests adjust amounts to edge values, add controls
Presigning sweeps takes change outputs into account.
Each primary deposit id of a sweep group points to
an optional change output.
sweepbatcher.presign scans all passed sweeps for
change outputs and passes them to constructUnsignedTx.
Optional change of a swap is encoded in its sweeps
as a pointer to the same change output. This change
is taken into account when constructing the unsigned
batch transaction when it comes to tx weight and
outputs.
AddSweep may not be called after getting the first confirmation, but feerate
updates are still needed in case of reorg.
Update test TestFeeRateGrows not to call AddSweep again and make sure feerate
is updated itself.
The reorg channel is now passed to RegisterSpendNtfn, and waiting for spend
notifications remains active even after the transaction receives its first
confirmation. The dedicated goroutine previously used to wait for the spend
is no longer needed, as we now handle both spend and potential reorg events
in the main event loop while the batch is running.
Following this change, RegisterConfirmationsNtfn runs without a reorg channel,
as it would only detect deep reorgs that undo the final confirmation - something
we can't handle anyway. We can't track fully confirmed swaps indefinitely to
guard against such rare reorgs; instead, we can mitigate the risk by increasing
the required confirmation depth.
Since bb837a4aec AddSweep loads sweeps so
one of possible errors is sql.ErrTxDone. Full error that is fixed:
> fetchSweeps failed: failed to load sweep 0000000000000000000101:1:
> failed to fetch sweep data for 010101000000:
> failed to fetch loop out for 010101000000:
> sql: transaction has already been committed or rolled back
For presigned possible remaining groups, the destination address of the current
batch was used instead of the destination address of an expected future batch.
TODO: reproduce in unit test "purged". For this, each swap should have a
separate destination address.
Method Presign is not as reliable as SignTx, because it checks transaction by
txid and can miss for example if LockTime is different. SignTx can do everything
Presign was used for.
There were two mistakes.
In case of a swap with multiple sweeps only the fee of the first sweep
of a swap was accounted.
Rounding diff (the remainder) was attributed to all the sweeps rather than to
the first (primary) sweep of the batch. The sweep to attribute the remainder
was chosen by comparing SignatureScript which is always empty. New approach is
to find the primary sweep and to compare its outpoint directly.
In case of a reorg sweeps should not go to another batch but stay in the current
batch until it is reorg-safely confirmed. Only after that the remaining sweeps
are re-added to another batch.
Field sweep.completed is now set to true only for reorg-safely confirmed sweeps.
In handleConf we now use batch.persist() (i.e. store.UpdateSweepBatch) instead
of ConfirmBatch, because we set not only Confirmed flag, but also batchTxid.
Added option WithSkippedTxns, which has one historical problematic tx by default.
Sweeps originating from these transactions are omitted when reading from DB.
loopdb: add column sweep_batches.cancelled and replaced DropBatch with
CancelBatch. It is needed, because sweep.batch_id is a foreign key to batch.
Changed StoreMock.InsertSweepBatch not to reuse batch_id. This is needed by
the test, which checks that new batch has fresh ID.
Add fields ConfChan and ConfErrChan to SpendNotifier type which is a part of
SweepRequest passed to AddSweep method.
This is needed to reuse confirmation notifications on the calling side the same
way it is done for spending notifications.
Previously, dbBatch had a State field (enum: Open, Closed, Confirmed), but in
the database it is represented as a boolean Confirmed. The Closed state was
stored the same way as Open. This wasn't an issue in practice, since an Open
batch is quickly transitioned to Closed after startup.
However, the in-memory mock stores plain dbBatch instances, leading to
inconsistent behavior between the mock and the real DB-backed store. This
commit updates dbBatch to match the database representation by replacing
the State field with a Confirmed boolean.
Function monitorSpendAndNotify used to cancel the context passed to
RegisterSpendNtfn right after starting the goroutine processing results.
Spend notifications were missed.
Now the context is canceled when the goroutine finishes.
