sweepbatcher: do not fail on restoring empty batches #754

bhandras · 2024-05-24T08:38:31Z

Previously storing an empty batch would make the batcher fail to start as spinning up a restored batch assumes that there's a primary sweep added already. As there's no point in spinning up such batch we can just skip over it.
Furthermore we'll ensure that we won't try to ever publish an empty batch to avoid setting the fee rate too early.

Pull Request Checklist

Update release_notes.md if your PR contains major features, breaking changes or bugfixes

GeorgeTsagk · 2024-05-24T09:27:36Z

sweepbatcher/sweep_batcher.go

 	dbSweeps, err := b.store.FetchBatchSweeps(ctx, batch.id)
 	if err != nil {
 		return err
 	}

 	if len(dbSweeps) == 0 {
-		return fmt.Errorf("batch %d has no sweeps", batch.id)
+		log.Infof("skipping restored batch %d as it has no sweeps",


maybe also delete from storage

GeorgeTsagk · 2024-05-24T09:29:02Z

sweepbatcher/sweep_batcher.go

@@ -321,7 +330,7 @@ func (b *Batcher) handleSweep(ctx context.Context, sweep *sweep,
 	// If one of the batches accepts the sweep, we provide it to that batch.
 	for _, batch := range b.batches {
 		accepted, err := batch.addSweep(ctx, sweep)
-		if err != nil && err != ErrBatchShuttingDown {
+		if err != nil {


we don't want to crash here, as it's possible that the batch confirmed by the time the sweep tries to enter it, returning an ErrBatchShuttingDown

it's not critical, but no need to cause an all-around system restart

could perhaps add some simple coverage for this, not sure how involved the unit test would be though

In the previous loop (trying to addSweep (update) to the batch to which it already belongs), the error from batch.addSweep is checked with regular err != nil check. Should the checks of errors from batch.addSweep in the two loops be the same?

Another question about that loop. Can we return earlier there, if the sweep was added successfuly?

diff --git a/sweepbatcher/sweep_batcher.go b/sweepbatcher/sweep_batcher.go index 0251101..6019f6c 100644 --- a/sweepbatcher/sweep_batcher.go +++ b/sweepbatcher/sweep_batcher.go @@ -388,6 +388,9 @@ func (b *Batcher) handleSweep(ctx context.Context, sweep *sweep, "accepted by batch %d", sweep.swapHash[:6], batch.id) } + + // The sweep was updated in the batch, our job is done. + return nil } }

Note that there's a check below for accepted and if that's true we return early.

In the previous loop (trying to addSweep (update) to the batch to which it already belongs), the error from batch.addSweep is checked with regular err != nil check

Oh, you're right. If the batch just confirmed it will trigger an ErrBatchShuttingDown which should not cause a daemon restart. In that case, we should do b.monitorSpendAndNotify in order to let the swap know that it was swept

Should this check also be if err != nil && !errors.Is(err, ErrBatchShuttingDown) {?

Added the error check to both code paths.

GeorgeTsagk · 2024-05-24T09:30:23Z

sweepbatcher/sweep_batcher_test.go

+
+	// Now make it quit by canceling the context.
+	cancel()
+	wg.Wait()


could go a bit further

add a sweep and verify that we now have 1 batch in memory, and 2 in storage

I've changed the test a bit since the dormant batch is now dropped from the db.

hieblmi · 2024-05-24T13:14:52Z

sweepbatcher/sweep_batch.go

@@ -493,7 +493,7 @@ func (b *batch) Run(ctx context.Context) error {
 			b.currentHeight = height

 		case <-timerChan:
-			if b.state == Open {
+			if b.state == Open && len(b.sweeps) > 0 {


maybe we can move the length check inside publish so that other calls to it also check for the length?

Good idea, done.

starius · 2024-05-24T13:37:16Z

sweepbatcher/sweep_batcher.go

@@ -458,6 +465,11 @@ func (b *Batcher) spinUpBatchFromDB(ctx context.Context, batch *batch) error {
 		log:              batchPrefixLogger(fmt.Sprintf("%d", batch.id)),
 	}

+	cfg := batchConfig{
+		maxTimeoutDistance: batch.cfg.maxTimeoutDistance,
+		batchConfTarget:    defaultBatchConfTarget,


Why setting defaultBatchConfTarget is needed here? If it is used, it means, that confTargets of sweeps were not taken into account.

The source of confTarget is fetchSweep method. I think, after resuming from DB we should make sure that confTarget is filled from fetchSweep before it is used to calculate fee rate (i.e. before publish). Then we can remove const defaultBatchConfTarget.

We update this when the first sweep comes in, see here

but agree it's basically a noop setting this here, as it's guaranteed it will be overwritten

Note that this is just a reorder, no added code.

yeah, ideally let's not introduce unwanted changes

starius · 2024-05-24T13:40:45Z

sweepbatcher/sweep_batcher_test.go

+
+	go func() {
+		defer wg.Done()
+		err = batcher.Run(ctx)


Let's name it to runErr just in case another err is created between this line and the check of the error in the end.

starius

LGTM 🏆

starius · 2024-05-24T16:32:44Z

sweepbatcher/store.go

@@ -108,6 +111,12 @@ func (s *SQLStore) InsertSweepBatch(ctx context.Context, batch *dbBatch) (int32,
 	return s.baseDb.InsertBatch(ctx, batchToInsertArgs(*batch))
 }

+// DropBatch drops a batch from the database. Note that we only use this call
+// for batches that have no sweeps and so we'd not be able to resume.
+func (s *SQLStore) DropBatch(ctx context.Context, id int32) error {


I propose to add a check that the number of sweep in the batch is 0. If the batch is not empty, return an error.

Good idea, added!

starius · 2024-05-24T16:35:28Z

sweepbatcher/sweep_batcher.go

@@ -321,7 +330,7 @@ func (b *Batcher) handleSweep(ctx context.Context, sweep *sweep,
 	// If one of the batches accepts the sweep, we provide it to that batch.
 	for _, batch := range b.batches {
 		accepted, err := batch.addSweep(ctx, sweep)
-		if err != nil && err != ErrBatchShuttingDown {
+		if err != nil {


Should this check also be if err != nil && !errors.Is(err, ErrBatchShuttingDown) {?

Previously storing an empty batch would make the batcher fail to start as spinning up a restored batch assumes that there's a primary sweep added already. As there's no point in spinning up such batch we can just skip over it. Furthermore we'll ensure that we won't try to ever publish an empty batch to avoid setting the fee rate too early.

hieblmi

LGTM!

It is always overwritten with primary sweep's confTarget. See lightninglabs#754 (comment)

It is always overwritten with primary sweep's confTarget. Print a warning if batchConfTarget is 0 in updateRbfRate. See lightninglabs#754 (comment)

bhandras requested review from GeorgeTsagk, starius and hieblmi May 24, 2024 08:38

bhandras force-pushed the sweepbatcher-empty-batch-fix branch 2 times, most recently from d18c3d4 to 6ea1536 Compare May 24, 2024 08:57

GeorgeTsagk reviewed May 24, 2024

View reviewed changes

bhandras force-pushed the sweepbatcher-empty-batch-fix branch from 6ea1536 to 12ef7d3 Compare May 24, 2024 11:45

hieblmi reviewed May 24, 2024

View reviewed changes

starius reviewed May 24, 2024

View reviewed changes

bhandras force-pushed the sweepbatcher-empty-batch-fix branch 4 times, most recently from ed9c22f to b0a1a90 Compare May 24, 2024 14:22

bhandras requested review from starius, GeorgeTsagk and hieblmi May 24, 2024 14:22

bhandras force-pushed the sweepbatcher-empty-batch-fix branch from b0a1a90 to 05f1afb Compare May 24, 2024 14:40

starius approved these changes May 24, 2024

View reviewed changes

bhandras force-pushed the sweepbatcher-empty-batch-fix branch from 05f1afb to df06885 Compare May 24, 2024 16:41

bhandras added 4 commits May 24, 2024 18:44

loopdb+sweepbatcher: add the DropBatch call

939c9b4

sweepbatcher: close the quit channel when the batcher is shutting down

e5ade6a

sweepbatcher: test that empty batches won't prevent startup

14de8f1

bhandras force-pushed the sweepbatcher-empty-batch-fix branch from df06885 to 14de8f1 Compare May 24, 2024 16:44

hieblmi approved these changes May 24, 2024

View reviewed changes

bhandras merged commit 563e7be into lightninglabs:master May 24, 2024
4 checks passed

bhandras deleted the sweepbatcher-empty-batch-fix branch May 24, 2024 17:04

starius added a commit to starius/loop that referenced this pull request May 29, 2024

sweepbatcher: remove const defaultBatchConfTarget

3268f0c

It is always overwritten with primary sweep's confTarget. See lightninglabs#754 (comment)

starius added a commit to starius/loop that referenced this pull request May 29, 2024

sweepbatcher: remove const defaultBatchConfTarget

2173dd5

It is always overwritten with primary sweep's confTarget. Print a warning if batchConfTarget is 0 in updateRbfRate. See lightninglabs#754 (comment)

starius added a commit to starius/loop that referenced this pull request May 30, 2024

sweepbatcher: remove const defaultBatchConfTarget

87fb185

It is always overwritten with primary sweep's confTarget. Print a warning if batchConfTarget is 0 in updateRbfRate. See lightninglabs#754 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sweepbatcher: do not fail on restoring empty batches #754

sweepbatcher: do not fail on restoring empty batches #754

bhandras commented May 24, 2024 •

edited

Loading

GeorgeTsagk May 24, 2024

bhandras May 24, 2024

GeorgeTsagk May 24, 2024

GeorgeTsagk May 24, 2024

starius May 24, 2024

bhandras May 24, 2024

GeorgeTsagk May 24, 2024

starius May 24, 2024

bhandras May 24, 2024

GeorgeTsagk May 24, 2024

bhandras May 24, 2024

bhandras May 24, 2024

hieblmi May 24, 2024

bhandras May 24, 2024

starius May 24, 2024

starius May 24, 2024

GeorgeTsagk May 24, 2024 •

edited

Loading

bhandras May 24, 2024

GeorgeTsagk May 24, 2024

starius May 24, 2024

bhandras May 24, 2024

starius left a comment

starius May 24, 2024

bhandras May 24, 2024

starius May 24, 2024

hieblmi left a comment

sweepbatcher: do not fail on restoring empty batches #754

sweepbatcher: do not fail on restoring empty batches #754

Conversation

bhandras commented May 24, 2024 • edited Loading

Pull Request Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeorgeTsagk May 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

starius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hieblmi left a comment

Choose a reason for hiding this comment

bhandras commented May 24, 2024 •

edited

Loading

GeorgeTsagk May 24, 2024 •

edited

Loading