From 40ad1ce609fb710be4520c2d9a2221dc2ceadb4e Mon Sep 17 00:00:00 2001 From: Boris Nagaev Date: Wed, 29 May 2024 15:07:32 -0300 Subject: [PATCH] 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.) --- sweepbatcher/sweep_batch.go | 19 ++++- sweepbatcher/sweep_batcher.go | 5 +- sweepbatcher/sweep_batcher_test.go | 120 ++++++++++++++++++++++++++++- 3 files changed, 139 insertions(+), 5 deletions(-) diff --git a/sweepbatcher/sweep_batch.go b/sweepbatcher/sweep_batch.go index 0aa5b4c..da27e30 100644 --- a/sweepbatcher/sweep_batch.go +++ b/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 diff --git a/sweepbatcher/sweep_batcher.go b/sweepbatcher/sweep_batcher.go index bc69d42..4c3a533 100644 --- a/sweepbatcher/sweep_batcher.go +++ b/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 diff --git a/sweepbatcher/sweep_batcher_test.go b/sweepbatcher/sweep_batcher_test.go index 67528c7..4b73cfa 100644 --- a/sweepbatcher/sweep_batcher_test.go +++ b/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) +}