chainntnfs/txnotifier: correctly update confirm/spend hints on chain updates

In this commit, we address an issue w.r.t. updating the confirm hints
for transactions and spend hints for outpoints on chain updates.
Previously, upon a block being disconnected, we'd attempt to commit a
new height hint for all outstanding confirmation notifications. This is
not correct because we'll end up modifying the height hint for things
that have confirmed at a previous height than the one being
disconnected. This would cause issues on restart when attempting a
historical dispatch, as we would start scanning at a height above which
the transaction actually confirmed in.

This has been addressed by only updating the hints for outstanding
notifications that are still unconfirmed/unspent and for notifications
that were confirmed/spent within the block being connected/disconnected.

chainntnfs/txnotifier.go

@@ -992,54 +992,11 @@ func (n *TxNotifier) ConnectTip(blockHash *chainhash.Hash, blockHeight uint32,
 		}
 	}
 
-	// In order to update the height hint for all the required transactions
+	// Now that we've determined which transactions were confirmed and which
-	// under one database transaction, we'll gather the set of unconfirmed
+	// outpoints were spent within the new block, we can update their
-	// transactions along with the ones that confirmed at the current
+	// entries in their respective caches, along with all of our unconfirmed
-	// height. To do so, we'll iterate over the confNotifications map, which
+	// transactions and unspent outpoints.
-	// contains the transactions we currently have notifications for. Since
+	n.updateHints(blockHeight)
-	// this map doesn't tell us whether the transaction has confirmed or
-	// not, we'll need to look at txsByInitialHeight to determine so.
-	var txsToUpdateHints []chainhash.Hash
-	for confirmedTx := range n.txsByInitialHeight[n.currentHeight] {
-		txsToUpdateHints = append(txsToUpdateHints, confirmedTx)
-	}
-out:
-	for maybeUnconfirmedTx, confSet := range n.confNotifications {
-		// We shouldn't update the confirm hints if we still have a
-		// pending rescan in progress. We'll skip writing any for
-		// notification sets that haven't reached rescanComplete.
-		if confSet.rescanStatus != rescanComplete {
-			continue
-		}
-
-		for height, confirmedTxs := range n.txsByInitialHeight {
-			// Skip the transactions that confirmed at the new block
-			// height as those have already been added.
-			if height == blockHeight {
-				continue
-			}
-
-			// If the transaction was found within the set of
-			// confirmed transactions at this height, we'll skip it.
-			if _, ok := confirmedTxs[maybeUnconfirmedTx]; ok {
-				continue out
-			}
-		}
-		txsToUpdateHints = append(txsToUpdateHints, maybeUnconfirmedTx)
-	}
-
-	if len(txsToUpdateHints) > 0 {
-		err := n.confirmHintCache.CommitConfirmHint(
-			n.currentHeight, txsToUpdateHints...,
-		)
-		if err != nil {
-			// The error is not fatal, so we should not return an
-			// error to the caller.
-			Log.Errorf("Unable to update confirm hint to %d for "+
-				"%v: %v", n.currentHeight, txsToUpdateHints,
-				err)
-		}
-	}
 
 	// Next, we'll dispatch an update to all of the notification clients for
 	// our watched transactions with the number of confirmations left at
@@ -1141,18 +1098,10 @@ func (n *TxNotifier) DisconnectTip(blockHeight uint32) error {
 	n.currentHeight--
 	n.reorgDepth++
 
-	// Rewind the height hint for all watched transactions.
+	// With the block disconnected, we'll update the confirm and spend hints
-	var txs []chainhash.Hash
+	// for our transactions and outpoints to reflect the new height, except
-	for tx := range n.confNotifications {
+	// for those that have confirmed/spent at previous heights.
-		txs = append(txs, tx)
+	n.updateHints(blockHeight)
-	}
-
-	err := n.confirmHintCache.CommitConfirmHint(n.currentHeight, txs...)
-	if err != nil {
-		Log.Errorf("Unable to update confirm hint to %d for %v: %v",
-			n.currentHeight, txs, err)
-		return err
-	}
 
 	// We'll go through all of our watched transactions and attempt to drain
 	// their notification channels to ensure sending notifications to the
@@ -1227,6 +1176,95 @@ func (n *TxNotifier) DisconnectTip(blockHeight uint32) error {
 	return nil
 }
 
+// updateHints attempts to update the confirm and spend hints for all relevant
+// transactions and outpoints respectively. The height parameter is used to
+// determine which transactions and outpoints we should update based on whether
+// a new block is being connected/disconnected.
+//
+// NOTE: This must be called with the TxNotifier's lock held and after its
+// height has already been reflected by a block being connected/disconnected.
+func (n *TxNotifier) updateHints(height uint32) {
+	// TODO(wilmer): update under one database transaction.
+	//
+	// To update the height hint for all the required transactions under one
+	// database transaction, we'll gather the set of unconfirmed
+	// transactions along with the ones that confirmed at the height being
+	// connected/disconnected.
+	txsToUpdateHints := n.unconfirmedTxs()
+	for confirmedTx := range n.txsByInitialHeight[height] {
+		txsToUpdateHints = append(txsToUpdateHints, confirmedTx)
+	}
+	err := n.confirmHintCache.CommitConfirmHint(
+		n.currentHeight, txsToUpdateHints...,
+	)
+	if err != nil {
+		// The error is not fatal as this is an optimistic optimization,
+		// so we'll avoid returning an error.
+		Log.Debugf("Unable to update confirm hints to %d for "+
+			"%v: %v", n.currentHeight, txsToUpdateHints, err)
+	}
+
+	// Similarly, to update the height hint for all the required outpoints
+	// under one database transaction, we'll gather the set of unspent
+	// outpoints along with the ones that were spent at the height being
+	// connected/disconnected.
+	opsToUpdateHints := n.unspentOutPoints()
+	for spentOp := range n.opsBySpendHeight[height] {
+		opsToUpdateHints = append(opsToUpdateHints, spentOp)
+	}
+	err = n.spendHintCache.CommitSpendHint(
+		n.currentHeight, opsToUpdateHints...,
+	)
+	if err != nil {
+		// The error is not fatal as this is an optimistic optimization,
+		// so we'll avoid returning an error.
+		Log.Debugf("Unable to update spend hints to %d for "+
+			"%v: %v", n.currentHeight, opsToUpdateHints, err)
+	}
+}
+
+// unconfirmedTxs returns the set of transactions that are still seen as
+// unconfirmed by the TxNotifier.
+//
+// NOTE: This method must be called with the TxNotifier's lock held.
+func (n *TxNotifier) unconfirmedTxs() []chainhash.Hash {
+	var unconfirmedTxs []chainhash.Hash
+	for tx, confNtfnSet := range n.confNotifications {
+		// If the notification is already aware of its confirmation
+		// details, or it's in the process of learning them, we'll skip
+		// it as we can't yet determine if it's confirmed or not.
+		if confNtfnSet.rescanStatus != rescanComplete ||
+			confNtfnSet.details != nil {
+			continue
+		}
+
+		unconfirmedTxs = append(unconfirmedTxs, tx)
+	}
+
+	return unconfirmedTxs
+}
+
+// unspentOutPoints returns the set of outpoints that are still seen as unspent
+// by the TxNotifier.
+//
+// NOTE: This method must be called with the TxNotifier's lock held.
+func (n *TxNotifier) unspentOutPoints() []wire.OutPoint {
+	var unspentOps []wire.OutPoint
+	for op, spendNtfnSet := range n.spendNotifications {
+		// If the notification is already aware of its spend details, or
+		// it's in the process of learning them, we'll skip it as we
+		// can't yet determine if it's unspent or not.
+		if spendNtfnSet.rescanStatus != rescanComplete ||
+			spendNtfnSet.details != nil {
+			continue
+		}
+
+		unspentOps = append(unspentOps, op)
+	}
+
+	return unspentOps
+}
+
 // dispatchConfReorg dispatches a reorg notification to the client if the
 // confirmation notification was already delivered.
 //