sweepbatcher: add options WithInitialDelay and WithPublishDelay #801
Conversation
starius
force-pushed
the
sweepbatcher-wait
branch
3 times, most recently
from
3d84565
to
40a9e29
Compare
sweepbatcher/sweep_batch.go
Outdated
| // skipBefore is the time before which we skip batch publishing. | ||
| // This is needed to facilitate better grouping of sweeps. | ||
| // For batches loaded from DB waitingPeriod should be 0. | ||
| skipBefore := time.Now().Add(b.cfg.waitingPeriod) |
There was a problem hiding this comment.
nit: instead of using real time, we could adopt the lnc/clock package so we can simulate time better in tests as needed.
There was a problem hiding this comment.
Done. I added this in a separate commit "sweepbatcher: use lnd/clock for timers".
While updating the test, I found that the batch starts empty and then the first sweep is added. This results in the order of RegisterSpendNtfn and timer registrations being nondeterministic. I decided to catch this in the test via two separate goroutines to avoid deadlocks. Maybe the batch can be reworked in the future make sure it always calls RegisterSpendNtfn before starting any timers, then the code of the test can be simplified.
Testing for urgent batch failed sometimes, because the check happen to run before addSweep and the batch was empty at that point (timeout unknown). To fix this, the check was moved to timerChan handler, just before the code that checks if waiting period has ended, before publishing. By that time the sweep is added.
Also I found that lnd/clock is imported and stored as a field in store.go, but not used. I removed it in a separate commit.
There was a problem hiding this comment.
Maybe the batch can be reworked in the future make sure it always calls RegisterSpendNtfn before starting any timers, then the code of the test can be simplified.
Agreed!
starius
force-pushed
the
sweepbatcher-wait
branch
from
40a9e29
to
7d34e2c
Compare
starius
force-pushed
the
sweepbatcher-wait
branch
from
7d34e2c
to
7b02f05
Compare
starius
changed the title
|
|
||
| startTime := time.Date(2018, 11, 1, 0, 0, 0, 0, time.UTC) | ||
| tickSignal := make(chan time.Duration) | ||
| testClock := clock.NewTestClockWithTickSignal(startTime, tickSignal) |
| wg.Wait() | ||
|
|
||
| // Make sure the batcher exited without an error. | ||
| checkBatcherError(t, runErr) |
Renamed defaultPublishDelay to defaultTestnetPublishDelay. Its godoc was already for defaultTestnetPublishDelay, but the const was named defaultPublishDelay.
Added option: WithClock. Use it for publishDelay timer. It will be used for testing.
WithInitialDelay instructs sweepbatcher to wait for the duration provided after new batch creation before it is first published. This facilitates better grouping. It only affects newly created batches, not batches loaded from DB, so publishing does happen in case of a daemon restart (especially important in case of a crashloop). Defaults to 0s. If a sweep is about to expire (time until timeout is less that 2x initialDelay), then waiting is skipped.
WithPublishDelay sets the delay of batch publishing that is applied in the beginning, after the appearance of a new block in the network or after the end of initial delay (see WithInitialDelay). It is needed to prevent unnecessary transaction publishments when a spend is detected on that block. Default value depends on the network: 5 seconds in mainnet, 0.5s in testnet. For batches recovered from DB this value is always 0s.
starius
force-pushed
the
sweepbatcher-wait
branch
from
7b02f05
to
e007b36
Compare
|
Rebased |
starius
force-pushed
the
sweepbatcher-wait
branch
from
e007b36
to
5b4a9c0
Compare
Tested scenarios: 1. For a regular sweep newly added it waits for initialDelay + publishDelay before publishing a transaction. 2. For a sweep recovered from DB it does not wait before publishing tx. 3. If a sweep is about to expire, initialDelay is skipped.
starius
force-pushed
the
sweepbatcher-wait
branch
from
5b4a9c0
to
9af6718
Compare
|
Fixed flakiness in new test. Fixed the code waiting for new sweep being added. |
WithInitialDelay instructs sweepbatcher to wait for the duration provided after new batch creation before it is first published. This facilitates better grouping. It only affects newly created batches, not batches loaded from DB, so publishing does happen in case of a daemon restart (especially important in case of a crashloop). Defaults to 0s. If a sweep is about to expire (time until timeout is less that 2x initialDelay), then waiting is skipped.
WithPublishDelay sets the delay of batch publishing that is applied in the beginning, after the appearance of a new block in the network or after the end of initial delay (see WithInitialDelay). It is needed to prevent unnecessary transaction publishments when a spend is detected on that block. Default value depends on the network: 5 seconds in mainnet, 0.5s in testnet. For batches recovered from DB this value is always 0s.
Also tested both options, in particular tested:
Pull Request Checklist
release_notes.mdif your PR contains major features, breaking changes or bugfixes