sweepbatcher: fix reorg detection #975
Conversation
starius
force-pushed
the
spend-reorgs
branch
2 times, most recently
from
7763774 to
c7f22b7
Compare
| @@ -979,23 +979,26 @@ func (b *batch) Run(ctx context.Context) error { | |||
| return fmt.Errorf("handleSpend error: %w", err) | |||
| } | |||
|
|
|||
| case err := <-b.spendErrChan: | |||
There was a problem hiding this comment.
unrelated to this, but since the Run method is quite long, could we move the code at the beginning for around initialDelay and initialDelayChan into a separate function?
There was a problem hiding this comment.
initialDelay, initialDelayChan and related variables in the beginning of Run are too connected to each other. In the current version they are all local and it is easy to follow the logic of waiting. How to factor out these variables without sacrificing readability?
I updated the last commit: factored out feerate updates to updateFeeRate method.
| spendCtx, &primarySweep.outpoint, primarySweep.htlc.PkScript, | ||
| reorgChan := make(chan struct{}, 1) | ||
|
|
||
| spendChan, spendErrChan, err := b.chainNotifier.RegisterSpendNtfn( |
There was a problem hiding this comment.
Will RegisterSpendNtfn keep spendChan and spendErrChan open and signal spends/errors after the reorg chan fired?
I assume yes, so this is a nice simplification of the spend-handling flow.
There was a problem hiding this comment.
Yes, I added an itest for LND to check this: lightningnetwork/lnd#10083
| } | ||
|
|
||
| // minimumSweepFeeRate determines minimum feerate for a sweep. | ||
| func minimumSweepFeeRate(ctx context.Context, customFeeRate FeeRateProvider, |
There was a problem hiding this comment.
great that you ensured that AddSweep and Run in the reorg case call the same fee rate estimates.
| @@ -10,7 +11,6 @@ import ( | |||
| "github.com/lightninglabs/lndclient" | |||
| "github.com/lightningnetwork/lnd/chainntnfs" | |||
| "github.com/lightningnetwork/lnd/lnrpc/chainrpc" | |||
| "golang.org/x/net/context" | |||
| case <-ctx.Done(): | ||
| } | ||
| }() | ||
| b.spendChan = spendChan |
There was a problem hiding this comment.
Nice simplification. I'd add a comment that this is safe to do as we're always calling monitorSpend from the runloop's goroutine.
There was a problem hiding this comment.
Thanks! I added the comment.
sweepbatcher/sweep_batcher.go
Outdated
| return minFeeRate, nil | ||
| } | ||
|
|
||
| if sweepConfTarget == 0 { |
There was a problem hiding this comment.
LND's walletkit will fail with conftarget of 0 or 1. A way around would be override it to 2 if smaller?
There was a problem hiding this comment.
I added this check in a separate commit. Note that this check is unlikely to fire, since this is already fixed in GetConfTargetAndFeeRate.
Since bb837a4 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
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.
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.
This is already done in GetConfTargetAndFeeRate. The fix is just a double check.
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,
RegisterConfirmationsNtfnruns 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.Also changed sweepbatcher to update feerate independently of
AddSweepas well.AddSweepmay not be called after getting the first confirmation, but feerate updates are still needed in case of reorg. Update testTestFeeRateGrowsnot to callAddSweepagain and make sure feerate is updated itself.Pull Request Checklist
release_notes.mdif your PR contains major features, breaking changes or bugfixes