Make sure to reload all outpoints during startup

[yyforyongyu]

This PR adds a new method OutputsToWatch to reload all the relevant outpoints. Previously we used UnspentOutputs, which can skip locked outputs and unmined spending txns.

Depends on,

• wtxmgr: add method OutputsToWatch #1003

[Roasbeef]

Perhaps we can do an intermediate refactor here with a private helper function that modulates if we check the locked and unmined credits or not.

[Roasbeef]

Potential diff:

diff --git a/.gitignore b/.gitignore
index 75963309..8dce5946 100644
--- a/.gitignore
+++ b/.gitignore
@@ -5,3 +5,4 @@ coverage.txt
 *.swp
 .vscode
 .DS_Store
+.aider*
diff --git a/wtxmgr/tx.go b/wtxmgr/tx.go
index 921351b0..b23e2f75 100644
--- a/wtxmgr/tx.go
+++ b/wtxmgr/tx.go
@@ -805,210 +805,193 @@ func (s *Store) rollback(ns walletdb.ReadWriteBucket, height int32) error {
 	return putMinedBalance(ns, minedBalance)
 }
 
-// OutputsToWatch returns a list of outputs to monitor during the wallet's
-// startup. The returned items are similar to UnspentOutputs, exccept the
-// locked outputs and unmined credits are also returned here. In addition, we
-// only set the field `OutPoint` and `PkScript` for the `Credit`, as these are
-// the only fields used during the rescan.
-func (s *Store) OutputsToWatch(ns walletdb.ReadBucket) ([]Credit, error) {
-	var (
-		unspent []Credit
-		op      wire.OutPoint
-		block   Block
-	)
+// fetchCredits retrieves credits from the store based on the provided filters.
+// It iterates over both mined (unspent) and unmined credits.
+//
+// Parameters:
+//   - ns: The database bucket to read from.
+//   - includeLocked: If true, credits locked by LockOutput are included.
+//   - includeSpentByUnmined: If true, credits spent by unmined transactions
+//     are included.
+//   - populateFullDetails: If true, all fields of the Credit struct are
+//     populated. Otherwise, only OutPoint and PkScript are populated.
+func (s *Store) fetchCredits(ns walletdb.ReadBucket, includeLocked bool,
+	includeSpentByUnmined bool, populateFullDetails bool) ([]Credit, error) {
+
+	var credits []Credit
+	now := s.clock.Now() // Cache current time for lock checks
+
+	// Iterate over mined unspent credits (bucketUnspent).
+	unspentBucket := ns.NestedReadBucket(bucketUnspent)
+	if unspentBucket != nil {
+		err := unspentBucket.ForEach(func(k, v []byte) error {
+			var op wire.OutPoint
+			err := readCanonicalOutPoint(k, &op)
+			if err != nil {
+				return err
+			}
 
-	err := ns.NestedReadBucket(bucketUnspent).ForEach(func(k,
-		v []byte) error {
+			// Check if locked, skip if necessary.
+			if !includeLocked {
+				_, _, isLocked := isLockedOutput(ns, op, now)
+				if isLocked {
+					return nil
+				}
+			}
 
-		err := readCanonicalOutPoint(k, &op)
-		if err != nil {
-			return err
-		}
+			// Check if spent by unmined, skip if necessary.
+			if !includeSpentByUnmined {
+				if existsRawUnminedInput(ns, k) != nil {
+					return nil
+				}
+			}
 
-		err = readUnspentBlock(v, &block)
-		if err != nil {
-			return err
-		}
+			// Fetch the transaction record to get PkScript and potentially
+			// other details.
+			var block Block
+			err = readUnspentBlock(v, &block)
+			if err != nil {
+				return err
+			}
+			rec, err := fetchTxRecord(ns, &op.Hash, &block)
+			if err != nil {
+				// Wrap the error for context.
+				return fmt.Errorf("unable to retrieve tx %v for mined credit: %w",
+					op.Hash, err)
+			}
+			txOut := rec.MsgTx.TxOut[op.Index]
 
-		// TODO(jrick): reading the entire transaction should be
-		// avoidable. Creating the credit only requires the output
-		// amount and pkScript.
-		rec, err := fetchTxRecord(ns, &op.Hash, &block)
-		if err != nil {
-			return fmt.Errorf("unable to retrieve tx %v: %w",
-				op.Hash, err)
-		}
+			cred := Credit{
+				OutPoint: op,
+				PkScript: txOut.PkScript,
+			}
 
-		txOut := rec.MsgTx.TxOut[op.Index]
-		cred := Credit{
-			OutPoint: op,
-			PkScript: txOut.PkScript,
-		}
-		unspent = append(unspent, cred)
+			// Populate full details if requested.
+			if populateFullDetails {
+				blockTime, err := fetchBlockTime(ns, block.Height)
+				if err != nil {
+					// Wrap the error for context.
+					return fmt.Errorf("unable to fetch block time for height %d: %w",
+						block.Height, err)
+				}
+				cred.BlockMeta = BlockMeta{
+					Block: block,
+					Time:  blockTime,
+				}
+				cred.Amount = btcutil.Amount(txOut.Value)
+				cred.Received = rec.Received
+				cred.FromCoinBase = blockchain.IsCoinBaseTx(&rec.MsgTx)
+			}
 
-		return nil
-	})
-	if err != nil {
-		if _, ok := err.(Error); ok {
-			return nil, err
+			credits = append(credits, cred)
+			return nil
+		})
+		if err != nil {
+			// Check if it's already a storeError, otherwise wrap it.
+			if _, ok := err.(Error); ok {
+				return nil, err
+			}
+			str := "failed iterating unspent bucket"
+			return nil, storeError(ErrDatabase, str, err)
 		}
-		str := "failed iterating unspent bucket"
-		return nil, storeError(ErrDatabase, str, err)
 	}
 
-	err = ns.NestedReadBucket(bucketUnminedCredits).ForEach(func(k,
-		v []byte) error {
-
-		if err := readCanonicalOutPoint(k, &op); err != nil {
-			return err
-		}
-
-		// TODO(jrick): Reading/parsing the entire transaction record
-		// just for the output amount and script can be avoided.
-		recVal := existsRawUnmined(ns, op.Hash[:])
-
-		var rec TxRecord
-		err = readRawTxRecord(&op.Hash, recVal, &rec)
-		if err != nil {
-			return fmt.Errorf("unable to retrieve raw tx%v: %w",
-				op.Hash, err)
-		}
+	// Iterate over unmined credits (bucketUnminedCredits).
+	unminedCreditsBucket := ns.NestedReadBucket(bucketUnminedCredits)
+	if unminedCreditsBucket != nil {
+		err := unminedCreditsBucket.ForEach(func(k, v []byte) error {
+			var op wire.OutPoint
+			if err := readCanonicalOutPoint(k, &op); err != nil {
+				return err
+			}
 
-		txOut := rec.MsgTx.TxOut[op.Index]
-		cred := Credit{
-			OutPoint: op,
-			PkScript: txOut.PkScript,
-		}
-		unspent = append(unspent, cred)
+			// Check if locked, skip if necessary.
+			if !includeLocked {
+				_, _, isLocked := isLockedOutput(ns, op, now)
+				if isLocked {
+					return nil
+				}
+			}
 
-		return nil
-	})
-	if err != nil {
-		if _, ok := err.(Error); ok {
-			return nil, err
-		}
-		str := "failed iterating unmined credits bucket"
-		return nil, storeError(ErrDatabase, str, err)
-	}
+			// Check if spent by unmined, skip if necessary.
+			if !includeSpentByUnmined {
+				if existsRawUnminedInput(ns, k) != nil {
+					return nil
+				}
+			}
 
-	return unspent, nil
-}
+			// Fetch the transaction record to get PkScript and potentially
+			// other details.
+			recVal := existsRawUnmined(ns, op.Hash[:])
+			// existsRawUnmined should always return a value for a key
+			// in bucketUnminedCredits, but check defensively.
+			if recVal == nil {
+				log.Warnf("Unmined credit %v points to non-existent "+
+					"unmined tx record %v", op, op.Hash)
+				// Skip this credit as its tx record is missing.
+				return nil
+			}
 
-// UnspentOutputs returns all unspent received transaction outputs.
-// The order is undefined.
-func (s *Store) UnspentOutputs(ns walletdb.ReadBucket) ([]Credit, error) {
-	var unspent []Credit
+			var rec TxRecord
+			err := readRawTxRecord(&op.Hash, recVal, &rec)
+			if err != nil {
+				// Wrap the error for context.
+				return fmt.Errorf("unable to retrieve raw tx %v for unmined credit: %w",
+					op.Hash, err)
+			}
+			txOut := rec.MsgTx.TxOut[op.Index]
 
-	var op wire.OutPoint
-	var block Block
-	err := ns.NestedReadBucket(bucketUnspent).ForEach(func(k, v []byte) error {
-		err := readCanonicalOutPoint(k, &op)
-		if err != nil {
-			return err
-		}
+			cred := Credit{
+				OutPoint: op,
+				PkScript: txOut.PkScript,
+			}
 
-		// Skip the output if it's locked.
-		_, _, isLocked := isLockedOutput(ns, op, s.clock.Now())
-		if isLocked {
-			return nil
-		}
+			// Populate full details if requested.
+			if populateFullDetails {
+				cred.BlockMeta = BlockMeta{
+					Block: Block{Height: -1}, // Unmined height
+				}
+				cred.Amount = btcutil.Amount(txOut.Value)
+				cred.Received = rec.Received
+				cred.FromCoinBase = blockchain.IsCoinBaseTx(&rec.MsgTx)
+			}
 
-		if existsRawUnminedInput(ns, k) != nil {
-			// Output is spent by an unmined transaction.
-			// Skip this k/v pair.
+			credits = append(credits, cred)
 			return nil
-		}
-
-		err = readUnspentBlock(v, &block)
-		if err != nil {
-			return err
-		}
-
-		blockTime, err := fetchBlockTime(ns, block.Height)
-		if err != nil {
-			return err
-		}
-		// TODO(jrick): reading the entire transaction should
-		// be avoidable.  Creating the credit only requires the
-		// output amount and pkScript.
-		rec, err := fetchTxRecord(ns, &op.Hash, &block)
+		})
 		if err != nil {
-			return fmt.Errorf("unable to retrieve transaction %v: "+
-				"%w", op.Hash, err)
-		}
-		txOut := rec.MsgTx.TxOut[op.Index]
-		cred := Credit{
-			OutPoint: op,
-			BlockMeta: BlockMeta{
-				Block: block,
-				Time:  blockTime,
-			},
-			Amount:       btcutil.Amount(txOut.Value),
-			PkScript:     txOut.PkScript,
-			Received:     rec.Received,
-			FromCoinBase: blockchain.IsCoinBaseTx(&rec.MsgTx),
-		}
-		unspent = append(unspent, cred)
-		return nil
-	})
-	if err != nil {
-		if _, ok := err.(Error); ok {
-			return nil, err
+			// Check if it's already a storeError, otherwise wrap it.
+			if _, ok := err.(Error); ok {
+				return nil, err
+			}
+			str := "failed iterating unmined credits bucket"
+			return nil, storeError(ErrDatabase, str, err)
 		}
-		str := "failed iterating unspent bucket"
-		return nil, storeError(ErrDatabase, str, err)
 	}
 
-	err = ns.NestedReadBucket(bucketUnminedCredits).ForEach(func(k, v []byte) error {
-		if err := readCanonicalOutPoint(k, &op); err != nil {
-			return err
-		}
-
-		// Skip the output if it's locked.
-		_, _, isLocked := isLockedOutput(ns, op, s.clock.Now())
-		if isLocked {
-			return nil
-		}
-
-		if existsRawUnminedInput(ns, k) != nil {
-			// Output is spent by an unmined transaction.
-			// Skip to next unmined credit.
-			return nil
-		}
-
-		// TODO(jrick): Reading/parsing the entire transaction record
-		// just for the output amount and script can be avoided.
-		recVal := existsRawUnmined(ns, op.Hash[:])
-		var rec TxRecord
-		err = readRawTxRecord(&op.Hash, recVal, &rec)
-		if err != nil {
-			return fmt.Errorf("unable to retrieve raw transaction "+
-				"%v: %w", op.Hash, err)
-		}
+	return credits, nil
+}
 
-		txOut := rec.MsgTx.TxOut[op.Index]
-		cred := Credit{
-			OutPoint: op,
-			BlockMeta: BlockMeta{
-				Block: Block{Height: -1},
-			},
-			Amount:       btcutil.Amount(txOut.Value),
-			PkScript:     txOut.PkScript,
-			Received:     rec.Received,
-			FromCoinBase: blockchain.IsCoinBaseTx(&rec.MsgTx),
-		}
-		unspent = append(unspent, cred)
-		return nil
-	})
-	if err != nil {
-		if _, ok := err.(Error); ok {
-			return nil, err
-		}
-		str := "failed iterating unmined credits bucket"
-		return nil, storeError(ErrDatabase, str, err)
-	}
+// OutputsToWatch returns a list of outputs to monitor during the wallet's
+// startup. The returned items are similar to UnspentOutputs, exccept the
+// locked outputs and unmined credits are also returned here. In addition, we
+// only set the field `OutPoint` and `PkScript` for the `Credit`, as these are
+// the only fields used during the rescan.
+func (s *Store) OutputsToWatch(ns walletdb.ReadBucket) ([]Credit, error) {
+	// OutputsToWatch needs all known outputs (mined and unmined),
+	// including locked ones and those spent by other unmined txs,
+	// but only requires minimal details (OutPoint, PkScript).
+	return s.fetchCredits(ns, true, true, false)
+}
 
-	return unspent, nil
+// UnspentOutputs returns all unspent received transaction outputs.
+// The order is undefined.
+func (s *Store) UnspentOutputs(ns walletdb.ReadBucket) ([]Credit, error) {
+	// UnspentOutputs needs outputs that are actually spendable:
+	// - Not locked.
+	// - Not spent by an unmined transaction.
+	// It requires full credit details.
+	return s.fetchCredits(ns, false, false, true)
 }
 
 // Balance returns the spendable wallet balance (total value of all unspent

[yyforyongyu]

Veeeery nice! Addressed them in #1003 since we need to merge that one first and then update the gomod here.

Think I will create a file similar to this one lightningnetwork/lnd#9696 so aider can use it as the convention file since it still has tiny issues like long line length (over 80).

[Roasbeef]

so aider can use it as the convention file since it still has tiny issues like long line length (over 80).

I find that at times you still need to do a brief manual pass depending on the model. I like the manual pass though, as it gives you a chance to review the code properly, and also make any other stylistic modifications.

[Roasbeef]

Can be rebased now.

[guggero]

One issue with the log, otherwise LGTM 🎉

[Roasbeef]

LGTM 🐛