sweepbatcher: load from DB preserves confTarget

It used to be set to default (defaultBatchConfTarget = 12) which
could in theory affect fee rate if updateRbfRate() and publish()
were not called before the batch was saved. (Unlikely scenario.)

Author: starius

sweepbatcher/sweep_batch.go

@@ -316,7 +316,22 @@ func NewBatch(cfg batchConfig, bk batchKit) *batch {
 }
 
 // NewBatchFromDB creates a new batch that already existed in storage.
-func NewBatchFromDB(cfg batchConfig, bk batchKit) *batch {
+func NewBatchFromDB(cfg batchConfig, bk batchKit) (*batch, error) {
+	// Make sure the batch is not empty.
+	if len(bk.sweeps) == 0 {
+		// This should never happen, as this precondition is already
+		// ensured in spinUpBatchFromDB.
+		return nil, fmt.Errorf("empty batch is not allowed")
+	}
+
+	// Assign batchConfTarget to primary sweep's confTarget.
+	for _, sweep := range bk.sweeps {
+		if sweep.swapHash == bk.primaryID {
+			cfg.batchConfTarget = sweep.confTarget
+			break
+		}
+	}
+
 	return &batch{
 		id:               bk.id,
 		state:            bk.state,
@@ -343,7 +358,7 @@ func NewBatchFromDB(cfg batchConfig, bk batchKit) *batch {
 		store:            bk.store,
 		log:              bk.log,
 		cfg:              &cfg,
-	}
+	}, nil
 }
 
 // addSweep tries to add a sweep to the batch. If this is the first sweep being

sweepbatcher/sweep_batcher.go

@@ -491,7 +491,10 @@ func (b *Batcher) spinUpBatchFromDB(ctx context.Context, batch *batch) error {
 		batchConfTarget:    defaultBatchConfTarget,
 	}
 
-	newBatch := NewBatchFromDB(cfg, batchKit)
+	newBatch, err := NewBatchFromDB(cfg, batchKit)
+	if err != nil {
+		return fmt.Errorf("failed in NewBatchFromDB: %w", err)
+	}
 
 	// We add the batch to our map of batches and start it.
 	b.batches[batch.id] = newBatch

sweepbatcher/sweep_batcher_test.go

@@ -1109,7 +1109,7 @@ func TestRestoringEmptyBatch(t *testing.T) {
 	require.NoError(t, err)
 	require.Len(t, batches, 1)
 
-	// Now make it quit by canceling the context.
+	// Now make the batcher quit by canceling the context.
 	cancel()
 	wg.Wait()
 
@@ -1297,9 +1297,125 @@ func TestHandleSweepTwice(t *testing.T) {
 	require.Equal(t, 1, len(batcher.batches[0].sweeps))
 	require.Equal(t, 1, len(batcher.batches[1].sweeps))
 
-	// Now make it quit by canceling the context.
+	// Now make the batcher quit by canceling the context.
 	cancel()
 	wg.Wait()
 
 	checkBatcherError(t, runErr)
 }
+
+// TestRestoringPreservesConfTarget tests that after the batch is written to DB
+// and loaded back, its batchConfTarget value is preserved.
+func TestRestoringPreservesConfTarget(t *testing.T) {
+	defer test.Guard(t)()
+
+	lnd := test.NewMockLnd()
+	ctx, cancel := context.WithCancel(context.Background())
+
+	store := loopdb.NewStoreMock(t)
+
+	batcherStore := NewStoreMock(store)
+
+	batcher := NewBatcher(lnd.WalletKit, lnd.ChainNotifier, lnd.Signer,
+		testMuSig2SignSweep, nil, lnd.ChainParams, batcherStore, store)
+
+	var wg sync.WaitGroup
+	wg.Add(1)
+
+	var runErr error
+	go func() {
+		defer wg.Done()
+		runErr = batcher.Run(ctx)
+	}()
+
+	// Wait for the batcher to be initialized.
+	<-batcher.initDone
+
+	// Create a sweep request.
+	sweepReq := SweepRequest{
+		SwapHash: lntypes.Hash{1, 1, 1},
+		Value:    111,
+		Outpoint: wire.OutPoint{
+			Hash:  chainhash.Hash{1, 1},
+			Index: 1,
+		},
+		Notifier: &dummyNotifier,
+	}
+
+	swap := &loopdb.LoopOutContract{
+		SwapContract: loopdb.SwapContract{
+			CltvExpiry:      111,
+			AmountRequested: 111,
+		},
+
+		SwapInvoice:     swapInvoice,
+		SweepConfTarget: 123,
+	}
+
+	err := store.CreateLoopOut(ctx, sweepReq.SwapHash, swap)
+	require.NoError(t, err)
+	store.AssertLoopOutStored()
+
+	// Deliver sweep request to batcher.
+	require.NoError(t, batcher.AddSweep(&sweepReq))
+
+	// Since a batch was created we check that it registered for its primary
+	// sweep's spend.
+	<-lnd.RegisterSpendChannel
+
+	// Once batcher receives sweep request it will eventually spin up a
+	// batch.
+	require.Eventually(t, func() bool {
+		// Make sure that the sweep was stored and we have exactly one
+		// active batch, with one sweep and proper batchConfTarget.
+		return batcherStore.AssertSweepStored(sweepReq.SwapHash) &&
+			len(batcher.batches) == 1 &&
+			len(batcher.batches[0].sweeps) == 1 &&
+			batcher.batches[0].cfg.batchConfTarget == 123
+	}, test.Timeout, eventuallyCheckFrequency)
+
+	// Make sure we have stored the batch.
+	batches, err := batcherStore.FetchUnconfirmedSweepBatches(ctx)
+	require.NoError(t, err)
+	require.Len(t, batches, 1)
+
+	// Now make the batcher quit by canceling the context.
+	cancel()
+	wg.Wait()
+
+	// Make sure the batcher exited without an error.
+	checkBatcherError(t, runErr)
+
+	// Now launch it again.
+	batcher = NewBatcher(lnd.WalletKit, lnd.ChainNotifier, lnd.Signer,
+		testMuSig2SignSweep, nil, lnd.ChainParams, batcherStore, store)
+	ctx, cancel = context.WithCancel(context.Background())
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		runErr = batcher.Run(ctx)
+	}()
+
+	// Wait for the batcher to be initialized.
+	<-batcher.initDone
+
+	// Wait for batch to load.
+	require.Eventually(t, func() bool {
+		return batcherStore.AssertSweepStored(sweepReq.SwapHash) &&
+			len(batcher.batches) == 1 &&
+			len(batcher.batches[0].sweeps) == 1
+	}, test.Timeout, eventuallyCheckFrequency)
+
+	// Make sure batchConfTarget was preserved.
+	require.Equal(t, 123, int(batcher.batches[0].cfg.batchConfTarget))
+
+	// Expect registration for spend notification.
+	<-lnd.RegisterSpendChannel
+
+	// Now make the batcher quit by canceling the context.
+	cancel()
+	wg.Wait()
+
+	// Make sure the batcher exited without an error.
+	checkBatcherError(t, runErr)
+}