From 353f208031e5047207f101ba2e956f504df3f657 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Wed, 12 Feb 2025 20:04:40 +0800 Subject: [PATCH] sweep: refactor `IsOurTx` to not return an error Before this commit, the only error returned from `IsOurTx` is when the root bucket was not created. In that case, we should consider the tx to be not found in our db, since technically our db is empty. A future PR may consider treating our wallet as the single source of truth and query the wallet instead to check for past sweeping txns. --- sweep/mock_test.go | 4 ++-- sweep/store.go | 16 +++++++++------- sweep/store_test.go | 9 +++------ sweep/sweeper.go | 32 ++++---------------------------- sweep/sweeper_test.go | 8 ++++---- 5 files changed, 22 insertions(+), 47 deletions(-) diff --git a/sweep/mock_test.go b/sweep/mock_test.go index f9471f22a..9312b7e28 100644 --- a/sweep/mock_test.go +++ b/sweep/mock_test.go @@ -24,10 +24,10 @@ func NewMockSweeperStore() *MockSweeperStore { } // IsOurTx determines whether a tx is published by us, based on its hash. -func (s *MockSweeperStore) IsOurTx(hash chainhash.Hash) (bool, error) { +func (s *MockSweeperStore) IsOurTx(hash chainhash.Hash) bool { args := s.Called(hash) - return args.Bool(0), args.Error(1) + return args.Bool(0) } // StoreTx stores a tx we are about to publish. diff --git a/sweep/store.go b/sweep/store.go index cfab66381..e3e97908b 100644 --- a/sweep/store.go +++ b/sweep/store.go @@ -121,7 +121,7 @@ func deserializeTxRecord(r io.Reader) (*TxRecord, error) { type SweeperStore interface { // IsOurTx determines whether a tx is published by us, based on its // hash. - IsOurTx(hash chainhash.Hash) (bool, error) + IsOurTx(hash chainhash.Hash) bool // StoreTx stores a tx hash we are about to publish. StoreTx(*TxRecord) error @@ -276,15 +276,17 @@ func (s *sweeperStore) StoreTx(tr *TxRecord) error { }, func() {}) } -// IsOurTx determines whether a tx is published by us, based on its -// hash. -func (s *sweeperStore) IsOurTx(hash chainhash.Hash) (bool, error) { +// IsOurTx determines whether a tx is published by us, based on its hash. +func (s *sweeperStore) IsOurTx(hash chainhash.Hash) bool { var ours bool err := kvdb.View(s.db, func(tx kvdb.RTx) error { txHashesBucket := tx.ReadBucket(txHashesBucketKey) + // If the root bucket cannot be found, we consider the tx to be + // not found in our db. if txHashesBucket == nil { - return errNoTxHashesBucket + log.Error("Tx hashes bucket not found in sweeper store") + return nil } ours = txHashesBucket.Get(hash[:]) != nil @@ -294,10 +296,10 @@ func (s *sweeperStore) IsOurTx(hash chainhash.Hash) (bool, error) { ours = false }) if err != nil { - return false, err + return false } - return ours, nil + return ours } // ListSweeps lists all the sweep transactions we have in the sweeper store. diff --git a/sweep/store_test.go b/sweep/store_test.go index ea65b0177..e9d4db125 100644 --- a/sweep/store_test.go +++ b/sweep/store_test.go @@ -57,18 +57,15 @@ func TestStore(t *testing.T) { require.NoError(t, err) // Assert that both txes are recognized as our own. - ours, err := store.IsOurTx(tx1.TxHash()) - require.NoError(t, err) + ours := store.IsOurTx(tx1.TxHash()) require.True(t, ours, "expected tx to be ours") - ours, err = store.IsOurTx(tx2.TxHash()) - require.NoError(t, err) + ours = store.IsOurTx(tx2.TxHash()) require.True(t, ours, "expected tx to be ours") // An different hash should be reported as not being ours. var unknownHash chainhash.Hash - ours, err = store.IsOurTx(unknownHash) - require.NoError(t, err) + ours = store.IsOurTx(unknownHash) require.False(t, ours, "expected tx to not be ours") txns, err := store.ListSweeps() diff --git a/sweep/sweeper.go b/sweep/sweeper.go index 730a75586..9684039e4 100644 --- a/sweep/sweeper.go +++ b/sweep/sweeper.go @@ -1394,12 +1394,7 @@ func (s *UtxoSweeper) handleExistingInput(input *sweepInputMessage, func (s *UtxoSweeper) handleInputSpent(spend *chainntnfs.SpendDetail) { // Query store to find out if we ever published this tx. spendHash := *spend.SpenderTxHash - isOurTx, err := s.cfg.Store.IsOurTx(spendHash) - if err != nil { - log.Errorf("cannot determine if tx %v is ours: %v", - spendHash, err) - return - } + isOurTx := s.cfg.Store.IsOurTx(spendHash) // If this isn't our transaction, it means someone else swept outputs // that we were attempting to sweep. This can happen for anchor outputs @@ -1879,22 +1874,7 @@ func (s *UtxoSweeper) handleBumpEvent(r *bumpResp) error { // NOTE: It is enough to check the txid because the sweeper will create // outpoints which solely belong to the internal LND wallet. func (s *UtxoSweeper) IsSweeperOutpoint(op wire.OutPoint) bool { - found, err := s.cfg.Store.IsOurTx(op.Hash) - // In case there is an error fetching the transaction details from the - // sweeper store we assume the outpoint is still used by the sweeper - // (worst case scenario). - // - // TODO(ziggie): Ensure that confirmed outpoints are deleted from the - // bucket. - if err != nil && !errors.Is(err, errNoTxHashesBucket) { - log.Errorf("failed to fetch info for outpoint(%v:%d) "+ - "with: %v, we assume it is still in use by the sweeper", - op.Hash, op.Index, err) - - return true - } - - return found + return s.cfg.Store.IsOurTx(op.Hash) } // markInputSwept marks the given input as swept by the tx. It will also notify @@ -1923,11 +1903,7 @@ func (s *UtxoSweeper) handleUnknownSpendTx(inp *SweeperInput, tx *wire.MsgTx) { op := inp.OutPoint() txid := tx.TxHash() - isOurTx, err := s.cfg.Store.IsOurTx(txid) - if err != nil { - log.Errorf("Cannot determine if tx %v is ours: %v", txid, err) - return - } + isOurTx := s.cfg.Store.IsOurTx(txid) // If this is our tx, it means it's a previous sweeping tx that got // confirmed, which could happen when a restart happens during the @@ -1955,7 +1931,7 @@ func (s *UtxoSweeper) handleUnknownSpendTx(inp *SweeperInput, tx *wire.MsgTx) { spentInputs[txIn.PreviousOutPoint] = struct{}{} } - err = s.removeConflictSweepDescendants(spentInputs) + err := s.removeConflictSweepDescendants(spentInputs) if err != nil { log.Warnf("unable to remove descendant transactions "+ "due to tx %v: ", txid) diff --git a/sweep/sweeper_test.go b/sweep/sweeper_test.go index ff48d9e8b..d1fbee1f6 100644 --- a/sweep/sweeper_test.go +++ b/sweep/sweeper_test.go @@ -1227,7 +1227,7 @@ func TestHandleUnknownSpendTxOurs(t *testing.T) { txid := tx.TxHash() // Mock the store to return true when calling IsOurTx. - store.On("IsOurTx", txid).Return(true, nil).Once() + store.On("IsOurTx", txid).Return(true).Once() // Call the method under test. s.handleUnknownSpendTx(si, tx) @@ -1271,7 +1271,7 @@ func TestHandleInputSpendTxThirdParty(t *testing.T) { txid := tx.TxHash() // Mock the store to return false when calling IsOurTx. - store.On("IsOurTx", txid).Return(false, nil).Once() + store.On("IsOurTx", txid).Return(false).Once() // Mock `ListSweeps` to return an empty slice as we are testing the // workflow here, not the method `removeConflictSweepDescendants`. @@ -1333,7 +1333,7 @@ func TestHandleBumpEventTxUnknownSpendNoRetry(t *testing.T) { } // Mock the store to return true when calling IsOurTx. - store.On("IsOurTx", txid).Return(true, nil).Once() + store.On("IsOurTx", txid).Return(true).Once() // Call the method under test. s.handleBumpEventTxUnknownSpend(resp) @@ -1419,7 +1419,7 @@ func TestHandleBumpEventTxUnknownSpendWithRetry(t *testing.T) { } // Mock the store to return true when calling IsOurTx. - store.On("IsOurTx", txid).Return(true, nil).Once() + store.On("IsOurTx", txid).Return(true).Once() // Mock the aggregator to return an empty slice as we are not testing // the actual sweeping behavior.