From 34e58006353aa55b2a664bde78a3f98f2dea26fd Mon Sep 17 00:00:00 2001
From: Dave Collins <davec@conformal.com>
Date: Tue, 13 Sep 2022 05:24:06 -0500
Subject: [PATCH 01/19] blockchain: Misc consistency cleanup pass.

This makes some minor and non-functional misc changes to improve code
consistency.
---
 internal/blockchain/chain.go                  |   3 +-
 .../blockchain/indexers/indexsubscriber.go    |   4 +-
 internal/blockchain/indexers/txindex_test.go  |   3 +-
 internal/blockchain/utxoviewpoint.go          |  54 ++++-----
 internal/blockchain/validate.go               | 111 +++++++++---------
 internal/blockchain/validate_test.go          |   2 +-
 6 files changed, 86 insertions(+), 91 deletions(-)

diff --git a/internal/blockchain/chain.go b/internal/blockchain/chain.go
index f22ae3789b..7c69c56158 100644
--- a/internal/blockchain/chain.go
+++ b/internal/blockchain/chain.go
@@ -940,7 +940,8 @@ func countSpentStakeOutputs(block *dcrutil.Block) int {
 			continue
 		}
 
-		// Exclude TreasuryBase and TSpend.
+		// Exclude treasurybase and treasury spends since neither have any
+		// inputs.
 		if stake.IsTreasuryBase(stx) || stake.IsTSpend(stx) {
 			continue
 		}
diff --git a/internal/blockchain/indexers/indexsubscriber.go b/internal/blockchain/indexers/indexsubscriber.go
index cf7ea665c2..d306378b38 100644
--- a/internal/blockchain/indexers/indexsubscriber.go
+++ b/internal/blockchain/indexers/indexsubscriber.go
@@ -243,8 +243,8 @@ func (s *IndexSubscriber) findLowestIndexTipHeight(queryer ChainQueryer) (int64,
 	return lowestHeight, bestHeight, nil
 }
 
-// CatchUp syncs all subscribed indexes to the the main chain by connecting
-// blocks from after the lowest index tip to the current main chain tip.
+// CatchUp syncs all subscribed indexes to the main chain by connecting blocks
+// from after the lowest index tip to the current main chain tip.
 //
 // This should be called after all indexes have subscribed for updates.
 func (s *IndexSubscriber) CatchUp(ctx context.Context, db database.DB, queryer ChainQueryer) error {
diff --git a/internal/blockchain/indexers/txindex_test.go b/internal/blockchain/indexers/txindex_test.go
index 084023b78e..b11088b4c7 100644
--- a/internal/blockchain/indexers/txindex_test.go
+++ b/internal/blockchain/indexers/txindex_test.go
@@ -104,8 +104,7 @@ func (tc *testChain) MainChainHasBlock(hash *chainhash.Hash) bool {
 	return ok
 }
 
-// Best returns the height and block hash of the the current
-// chain tip.
+// Best returns the height and block hash of the current chain tip.
 func (tc *testChain) Best() (int64, *chainhash.Hash) {
 	tc.mtx.Lock()
 	defer tc.mtx.Unlock()
diff --git a/internal/blockchain/utxoviewpoint.go b/internal/blockchain/utxoviewpoint.go
index 7f9b38fd72..1e269439b2 100644
--- a/internal/blockchain/utxoviewpoint.go
+++ b/internal/blockchain/utxoviewpoint.go
@@ -51,6 +51,13 @@ func (view *UtxoViewpoint) LookupEntry(outpoint wire.OutPoint) *UtxoEntry {
 	return view.entries[outpoint]
 }
 
+// RemoveEntry removes the given transaction output from the current state of
+// the view.  It will have no effect if the passed output does not exist in the
+// view.
+func (view *UtxoViewpoint) RemoveEntry(outpoint wire.OutPoint) {
+	delete(view.entries, outpoint)
+}
+
 // addTxOut adds the specified output to the view if it is not provably
 // unspendable.  When the view already has an entry for the output, it will be
 // marked unspent.  All fields will be updated for existing entries since it's
@@ -149,9 +156,8 @@ func (view *UtxoViewpoint) AddTxOut(tx *dcrutil.Tx, txOutIdx uint32,
 func (view *UtxoViewpoint) AddTxOuts(tx *dcrutil.Tx, blockHeight int64,
 	blockIndex uint32, isTreasuryEnabled bool) {
 
-	msgTx := tx.MsgTx()
-
 	// Set encoded flags for the transaction.
+	msgTx := tx.MsgTx()
 	isCoinBase := standalone.IsCoinBaseTx(msgTx, isTreasuryEnabled)
 	hasExpiry := msgTx.Expiry != wire.NoExpiryValue
 	txType := stake.DetermineTxType(msgTx)
@@ -234,18 +240,15 @@ func (view *UtxoViewpoint) connectTransaction(tx *dcrutil.Tx, blockHeight int64,
 	// Spend the referenced utxos by marking them spent in the view and, if a
 	// slice was provided for the spent txout details, append an entry to it.
 	isVote := stake.IsSSGen(msgTx)
-	var isTSpend bool
-	if isTreasuryEnabled {
-		isTSpend = stake.IsTSpend(msgTx)
-	}
+	isTreasurySpend := isTreasuryEnabled && stake.IsTSpend(msgTx)
 	for txInIdx, txIn := range msgTx.TxIn {
 		// Ignore stakebase since it has no input.
 		if isVote && txInIdx == 0 {
 			continue
 		}
 
-		// Ignore TSpend since it has no input.
-		if isTSpend {
+		// Ignore treasury spends since they have no inputs.
+		if isTreasurySpend {
 			continue
 		}
 
@@ -315,12 +318,11 @@ func (view *UtxoViewpoint) disconnectTransactions(block *dcrutil.Block,
 		}
 		isVote := txType == stake.TxTypeSSGen
 
-		var isTreasuryBase, isTSpend bool
-
+		var isTreasuryBase, isTreasurySpend bool
 		if isTreasuryEnabled {
-			isTSpend = txType == stake.TxTypeTSpend && stakeTree
-			isTreasuryBase = txType == stake.TxTypeTreasuryBase &&
-				stakeTree && txIdx == 0
+			isTreasurySpend = txType == stake.TxTypeTSpend && stakeTree
+			isTreasuryBase = txType == stake.TxTypeTreasuryBase && stakeTree &&
+				txIdx == 0
 		}
 
 		tree := wire.TxTreeRegular
@@ -387,10 +389,10 @@ func (view *UtxoViewpoint) disconnectTransactions(block *dcrutil.Block,
 		}
 
 		// Loop backwards through all of the transaction inputs (except for the
-		// coinbase and treasurybase which have no inputs) and unspend the
-		// referenced txos.  This is necessary to match the order of the spent
-		// txout entries.
-		if isCoinBase || isTreasuryBase || isTSpend {
+		// coinbase, treasurybase, and treasury spends which have no inputs) and
+		// unspend the referenced txos.  This is necessary to match the order of
+		// the spent txout entries.
+		if isCoinBase || isTreasuryBase || isTreasurySpend {
 			continue
 		}
 		for txInIdx := len(msgTx.TxIn) - 1; txInIdx > -1; txInIdx-- {
@@ -435,13 +437,6 @@ func (view *UtxoViewpoint) disconnectTransactions(block *dcrutil.Block,
 	return nil
 }
 
-// RemoveEntry removes the given transaction output from the current state of
-// the view.  It will have no effect if the passed output does not exist in the
-// view.
-func (view *UtxoViewpoint) RemoveEntry(outpoint wire.OutPoint) {
-	delete(view.entries, outpoint)
-}
-
 // disconnectRegularTransactions updates the view by removing all utxos created
 // by the transactions in regular tree of the provided block and unspending all
 // of the txos spent by those same transactions by using the provided spent txo
@@ -520,8 +515,7 @@ func (view *UtxoViewpoint) connectBlock(db database.DB, block, parent *dcrutil.B
 	// Disconnect the transactions in the regular tree of the parent block if
 	// the passed block disapproves it.
 	if !headerApprovesParent(&block.MsgBlock().Header) {
-		err := view.disconnectDisapprovedBlock(db, parent,
-			isTreasuryEnabled)
+		err := view.disconnectDisapprovedBlock(db, parent, isTreasuryEnabled)
 		if err != nil {
 			return err
 		}
@@ -637,8 +631,8 @@ func (view *UtxoViewpoint) Entries() map[wire.OutPoint]*UtxoEntry {
 	return view.entries
 }
 
-// ViewFilteredSet represents a set of utxos to fetch from the database that are
-// not already in a view.
+// ViewFilteredSet represents a set of utxos to fetch from the cache/backend
+// that are not already in a view.
 type ViewFilteredSet map[wire.OutPoint]struct{}
 
 // add conditionally adds the provided outpoint to the set if it does not
@@ -703,8 +697,8 @@ func (view *UtxoViewpoint) addRegularInputUtxos(block *dcrutil.Block,
 				i >= inFlightIndex {
 
 				originTx := regularTxns[inFlightIndex]
-				view.AddTxOuts(originTx, block.Height(),
-					uint32(inFlightIndex), isTreasuryEnabled)
+				view.AddTxOuts(originTx, block.Height(), uint32(inFlightIndex),
+					isTreasuryEnabled)
 				continue
 			}
 
diff --git a/internal/blockchain/validate.go b/internal/blockchain/validate.go
index d7bb21b228..19dbf498f6 100644
--- a/internal/blockchain/validate.go
+++ b/internal/blockchain/validate.go
@@ -95,8 +95,8 @@ const (
 	// checkForDuplicateHashes checks for duplicate hashes when validating
 	// blocks.  Because of the rule inserting the height into the second (nonce)
 	// txOut, there should never be a duplicate transaction hash that overwrites
-	// another. However, because there is a 2^128 chance of a collision, the
-	// paranoid user may wish to turn this feature on.
+	// another. However, because there is a 1 in 2^128 chance of a collision,
+	// the paranoid user may wish to turn this feature on.
 	checkForDuplicateHashes = false
 
 	// testNet3MaxDiffActivationHeight is the height that enforcement of the
@@ -863,10 +863,8 @@ func checkBlockSanity(block *dcrutil.Block, timeSource MedianTimeSource, flags B
 	// Do some preliminary checks on each regular transaction to ensure they
 	// are sane.
 	maxTxSize := uint64(chainParams.MaxTxSize)
-	transactions := block.Transactions()
-	for _, tx := range transactions {
-		msgTx := tx.MsgTx()
-		err := standalone.CheckTransactionSanity(msgTx, maxTxSize)
+	for _, tx := range msgBlock.Transactions {
+		err := standalone.CheckTransactionSanity(tx, maxTxSize)
 		if err != nil {
 			return standaloneToChainRuleError(err)
 		}
@@ -898,8 +896,9 @@ func checkBlockSanity(block *dcrutil.Block, timeSource MedianTimeSource, flags B
 
 	// Check for duplicate transactions.
 	existingTxHashes := make(map[chainhash.Hash]struct{})
+	regularTransactions := block.Transactions()
 	stakeTransactions := block.STransactions()
-	allTransactions := append(transactions, stakeTransactions...)
+	allTransactions := append(regularTransactions, stakeTransactions...)
 	for _, tx := range allTransactions {
 		hash := tx.Hash()
 		if _, exists := existingTxHashes[*hash]; exists {
@@ -3256,20 +3255,19 @@ func CountP2SHSigOps(tx *dcrutil.Tx, isCoinBaseTx bool, isStakeBaseTx bool, view
 // checkNumSigOps Checks the number of P2SH signature operations to make
 // sure they don't overflow the limits.  It takes a cumulative number of sig
 // ops as an argument and increments will each call.
-// TxTree true == Regular, false == Stake
-func checkNumSigOps(tx *dcrutil.Tx, view *UtxoViewpoint, index int, txTree bool, cumulativeSigOps int, isTreasuryEnabled bool) (int, error) {
+func checkNumSigOps(tx *dcrutil.Tx, view *UtxoViewpoint, index int, stakeTree bool, cumulativeSigOps int, isTreasuryEnabled bool) (int, error) {
 	msgTx := tx.MsgTx()
 	isSSGen := stake.IsSSGen(msgTx)
-	numsigOps := CountSigOps(tx, (index == 0) && txTree, isSSGen,
-		isTreasuryEnabled)
+	isCoinbaseTx := (index == 0) && !stakeTree
+	numsigOps := CountSigOps(tx, isCoinbaseTx, isSSGen, isTreasuryEnabled)
 
 	// Since the first (and only the first) transaction has already been
 	// verified to be a coinbase transaction, use (i == 0) && TxTree as an
 	// optimization for the flag to countP2SHSigOps for whether or not the
 	// transaction is a coinbase transaction rather than having to do a
 	// full coinbase check again.
-	numP2SHSigOps, err := CountP2SHSigOps(tx, (index == 0) && txTree,
-		isSSGen, view, isTreasuryEnabled)
+	numP2SHSigOps, err := CountP2SHSigOps(tx, isCoinbaseTx, isSSGen, view,
+		isTreasuryEnabled)
 	if err != nil {
 		log.Tracef("CountP2SHSigOps failed; error returned %v", err)
 		return 0, err
@@ -3372,7 +3370,7 @@ func getStakeBaseAmounts(txs []*dcrutil.Tx, view *UtxoViewpoint, isTreasuryEnabl
 }
 
 // getStakeTreeFees determines the amount of fees for in the stake tx tree of
-// some node given a transaction store.
+// some node given a utxo view.
 func getStakeTreeFees(subsidyCache *standalone.SubsidyCache, height int64,
 	txs []*dcrutil.Tx, view *UtxoViewpoint, isTreasuryEnabled,
 	isSubsidySplitEnabled bool) (dcrutil.Amount, error) {
@@ -3382,11 +3380,8 @@ func getStakeTreeFees(subsidyCache *standalone.SubsidyCache, height int64,
 	for _, tx := range txs {
 		msgTx := tx.MsgTx()
 		isSSGen := stake.IsSSGen(msgTx)
-		var isTSpend, isTreasuryBase bool
-		if isTreasuryEnabled {
-			isTSpend = stake.IsTSpend(msgTx)
-			isTreasuryBase = stake.IsTreasuryBase(msgTx)
-		}
+		isTreasuryBase := isTreasuryEnabled && stake.IsTreasuryBase(msgTx)
+		isTreasurySpend := isTreasuryEnabled && stake.IsTSpend(msgTx)
 
 		for i, in := range msgTx.TxIn {
 			// Ignore stakebases.
@@ -3394,13 +3389,9 @@ func getStakeTreeFees(subsidyCache *standalone.SubsidyCache, height int64,
 				continue
 			}
 
-			// Ignore TSpend.
-			if isTSpend && i == 0 {
-				continue
-			}
-
-			// Ignore treasury base.
-			if isTreasuryBase && i == 0 {
+			// Ignore treasury spends and treasurybases since they have no
+			// inputs.
+			if isTreasuryBase || isTreasurySpend {
 				continue
 			}
 
@@ -3431,7 +3422,7 @@ func getStakeTreeFees(subsidyCache *standalone.SubsidyCache, height int64,
 				isSubsidySplitEnabled)
 		}
 
-		if isTSpend {
+		if isTreasurySpend {
 			totalOutputs -= msgTx.TxIn[0].ValueIn
 		}
 
@@ -3450,10 +3441,10 @@ func getStakeTreeFees(subsidyCache *standalone.SubsidyCache, height int64,
 }
 
 // checkTransactionsAndConnect is the local function used to check the
-// transaction inputs for a transaction list given a predetermined TxStore.
+// transaction inputs for a transaction list given a predetermined utxo view.
 // After ensuring the transaction is valid, the transaction is connected to the
-// UTXO viewpoint.  TxTree true == Regular, false == Stake
-func (b *BlockChain) checkTransactionsAndConnect(inputFees dcrutil.Amount, node *blockNode, txs []*dcrutil.Tx, view *UtxoViewpoint, stxos *[]spentTxOut, txTree bool) error {
+// utxo view.
+func (b *BlockChain) checkTransactionsAndConnect(inputFees dcrutil.Amount, node *blockNode, txs []*dcrutil.Tx, view *UtxoViewpoint, stxos *[]spentTxOut, stakeTree bool) error {
 	isTreasuryEnabled, err := b.isTreasuryAgendaActive(node.parent)
 	if err != nil {
 		return err
@@ -3487,21 +3478,27 @@ func (b *BlockChain) checkTransactionsAndConnect(inputFees dcrutil.Amount, node
 		// Ensure that the number of signature operations is not beyond
 		// the consensus limit.
 		var err error
-		cumulativeSigOps, err = checkNumSigOps(tx, view, idx, txTree,
+		cumulativeSigOps, err = checkNumSigOps(tx, view, idx, stakeTree,
 			cumulativeSigOps, isTreasuryEnabled)
 		if err != nil {
 			return err
 		}
 
-		// This step modifies the txStore and marks the tx outs used
-		// spent, so be aware of this.
-		txFee, err := CheckTransactionInputs(b.subsidyCache, tx,
-			node.height, view, true, /* check fraud proofs */
-			b.chainParams, &prevHeader, isTreasuryEnabled,
-			isAutoRevocationsEnabled, isSubsidySplitEnabled)
+		// Perform a series of checks on the inputs to the transaction to ensure
+		// they are valid and calculate the total fees for it.
+		//
+		// An example of some of the checks include verifying all inputs exist,
+		// ensuring the coinbase seasoning requirements are met, detecting
+		// double spends, validating all values and fees are in the legal range,
+		// the total output amount doesn't exceed the input amount, and
+		// verifying the signatures to prove the spender was the owner of the
+		// coins and therefore allowed to spend them.
+		const checkFraudProof = true
+		txFee, err := CheckTransactionInputs(b.subsidyCache, tx, node.height,
+			view, checkFraudProof, b.chainParams, &prevHeader,
+			isTreasuryEnabled, isAutoRevocationsEnabled, isSubsidySplitEnabled)
 		if err != nil {
-			log.Tracef("CheckTransactionInputs failed; error "+
-				"returned: %v", err)
+			log.Tracef("CheckTransactionInputs failed; error returned: %v", err)
 			return err
 		}
 
@@ -3514,10 +3511,14 @@ func (b *BlockChain) checkTransactionsAndConnect(inputFees dcrutil.Amount, node
 				"overflows accumulator")
 		}
 
-		// Connect the transaction to the UTXO viewpoint, so that in
-		// flight transactions may correctly validate.
-		err = view.connectTransaction(tx, node.height, uint32(idx),
-			stxos, isTreasuryEnabled)
+		// Update the view to mark all utxos spent by the transaction as spent
+		// and add all of the outputs for this transaction which are not
+		// provably unspendable as available utxos.
+		//
+		// Also, update the passed spent txos slice to contain an entry for each
+		// output the transaction spends.
+		err = view.connectTransaction(tx, node.height, uint32(idx), stxos,
+			isTreasuryEnabled)
 		if err != nil {
 			return err
 		}
@@ -3528,7 +3529,7 @@ func (b *BlockChain) checkTransactionsAndConnect(inputFees dcrutil.Amount, node
 	// mining the block.  It is safe to ignore overflow and out of range
 	// errors here because those error conditions would have already been
 	// caught by the transaction sanity checks.
-	if txTree { //TxTreeRegular
+	if !stakeTree { //TxTreeRegular
 		// Apply penalty to fees if we're at stake validation height.
 		if node.height >= b.chainParams.StakeValidationHeight {
 			totalFees *= int64(node.voters)
@@ -3859,8 +3860,7 @@ func (b *BlockChain) checkConnectBlock(node *blockNode, block, parent *dcrutil.B
 	// Disconnect all of the transactions in the regular transaction tree of
 	// the parent if the block being checked votes against it.
 	if node.height > 1 && !voteBitsApproveParent(node.voteBits) {
-		err = view.disconnectDisapprovedBlock(b.db, parent,
-			isTreasuryEnabled)
+		err = view.disconnectDisapprovedBlock(b.db, parent, isTreasuryEnabled)
 		if err != nil {
 			return err
 		}
@@ -3870,7 +3870,7 @@ func (b *BlockChain) checkConnectBlock(node *blockNode, block, parent *dcrutil.B
 	// that 'overwrite' older transactions which are not fully spent.
 	err = b.checkDupTxs(block.STransactions(), view, wire.TxTreeStake)
 	if err != nil {
-		log.Tracef("checkDupTxs failed for cur TxTreeStake: %v", err)
+		log.Tracef("checkDupTxs failed for stake tree: %v", err)
 		return err
 	}
 
@@ -3884,11 +3884,11 @@ func (b *BlockChain) checkConnectBlock(node *blockNode, block, parent *dcrutil.B
 		return err
 	}
 
+	const stakeTreeTrue = true
 	err = b.checkTransactionsAndConnect(0, node, block.STransactions(),
-		view, stxos, false)
+		view, stxos, stakeTreeTrue)
 	if err != nil {
-		log.Tracef("checkTransactionsAndConnect failed for "+
-			"TxTreeStake: %v", err)
+		log.Tracef("checkTransactionsAndConnect failed for stake tree: %v", err)
 		return err
 	}
 
@@ -3899,7 +3899,7 @@ func (b *BlockChain) checkConnectBlock(node *blockNode, block, parent *dcrutil.B
 	stakeTreeFees, err := getStakeTreeFees(b.subsidyCache, node.height,
 		block.STransactions(), view, isTreasuryEnabled, isSubsidySplitEnabled)
 	if err != nil {
-		log.Tracef("getStakeTreeFees failed for TxTreeStake: %v", err)
+		log.Tracef("getStakeTreeFees failed for stake tree: %v", err)
 		return err
 	}
 
@@ -3947,15 +3947,16 @@ func (b *BlockChain) checkConnectBlock(node *blockNode, block, parent *dcrutil.B
 	// that 'overwrite' older transactions which are not fully spent.
 	err = b.checkDupTxs(block.Transactions(), view, wire.TxTreeRegular)
 	if err != nil {
-		log.Tracef("checkDupTxs failed for cur TxTreeRegular: %v", err)
+		log.Tracef("checkDupTxs failed for cur regular tree: %v", err)
 		return err
 	}
 
+	const stakeTreeFalse = false
 	err = b.checkTransactionsAndConnect(stakeTreeFees, node,
-		block.Transactions(), view, stxos, true)
+		block.Transactions(), view, stxos, stakeTreeFalse)
 	if err != nil {
-		log.Tracef("checkTransactionsAndConnect failed for cur "+
-			"TxTreeRegular: %v", err)
+		log.Tracef("checkTransactionsAndConnect failed for regular tree: %v",
+			err)
 		return err
 	}
 
diff --git a/internal/blockchain/validate_test.go b/internal/blockchain/validate_test.go
index 65ad48a6e4..51b3f61edf 100644
--- a/internal/blockchain/validate_test.go
+++ b/internal/blockchain/validate_test.go
@@ -1972,7 +1972,7 @@ func TestModifiedSubsidySplitSemantics(t *testing.T) {
 		trsySubsidy := cache.CalcTreasurySubsidy(height, numVotes, noTreasury)
 		powSubsidy := cache.CalcWorkSubsidyV2(height, numVotes, withDCP0010)
 
-		// Update the input value to the the new expected subsidy sum.
+		// Update the input value to the new expected subsidy sum.
 		coinbaseTx := b.Transactions[0]
 		coinbaseTx.TxIn[0].ValueIn = trsySubsidy + powSubsidy
 

From ac6c5968ff57c395a9c1ddab669b5f439b4ef1f8 Mon Sep 17 00:00:00 2001
From: Dave Collins <davec@conformal.com>
Date: Tue, 13 Sep 2022 05:24:07 -0500
Subject: [PATCH 02/19] blockchain: Pre-allocate in-flight utxoview tx map.

This modifies the code that tracks the in-flight transactions when
determining the input utxos that need to be loaded to pre-allocate the
map to avoid some additional overhead associated with growing the map
multiple times when a lot of transactions are involved.
---
 internal/blockchain/utxoviewpoint.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/internal/blockchain/utxoviewpoint.go b/internal/blockchain/utxoviewpoint.go
index 1e269439b2..25d35bcb7d 100644
--- a/internal/blockchain/utxoviewpoint.go
+++ b/internal/blockchain/utxoviewpoint.go
@@ -670,8 +670,8 @@ func (view *UtxoViewpoint) addRegularInputUtxos(block *dcrutil.Block,
 	// Build a map of in-flight transactions because some of the inputs in the
 	// regular transaction tree of this block could be referencing other
 	// transactions earlier in the block which are not yet in the chain.
-	txInFlight := map[chainhash.Hash]int{}
 	regularTxns := block.Transactions()
+	txInFlight := make(map[chainhash.Hash]int, len(regularTxns))
 	for i, tx := range regularTxns {
 		txInFlight[*tx.Hash()] = i
 	}

From 614ff3a2b65fb54d4eae9699d9b9ba947657d16a Mon Sep 17 00:00:00 2001
From: Dave Collins <davec@conformal.com>
Date: Tue, 13 Sep 2022 05:24:07 -0500
Subject: [PATCH 03/19] blockchain: Remove exported utxo cache SpendEntry.

This removes the exported SpendEntry method from the UtxoCache type to
help ensure cache coherency by forcing all external updates to the cache
to happen via Commit which the cache fully controls.
---
 internal/blockchain/utxocache.go      | 9 ---------
 internal/blockchain/utxocache_test.go | 2 +-
 2 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/internal/blockchain/utxocache.go b/internal/blockchain/utxocache.go
index d53449101d..69cdc9776a 100644
--- a/internal/blockchain/utxocache.go
+++ b/internal/blockchain/utxocache.go
@@ -334,15 +334,6 @@ func (c *UtxoCache) spendEntry(outpoint wire.OutPoint) {
 	cachedEntry.Spend()
 }
 
-// SpendEntry marks the specified output as spent.
-//
-// This function is safe for concurrent access.
-func (c *UtxoCache) SpendEntry(outpoint wire.OutPoint) {
-	c.cacheLock.Lock()
-	c.spendEntry(outpoint)
-	c.cacheLock.Unlock()
-}
-
 // fetchEntry returns the specified transaction output from the utxo set.  If
 // the output exists in the cache, it is returned immediately.  Otherwise, it
 // fetches the output from the backend, caches it, and returns it to the
diff --git a/internal/blockchain/utxocache_test.go b/internal/blockchain/utxocache_test.go
index e38ed5d41f..796705d968 100644
--- a/internal/blockchain/utxocache_test.go
+++ b/internal/blockchain/utxocache_test.go
@@ -437,7 +437,7 @@ func TestSpendEntry(t *testing.T) {
 		}
 
 		// Spend the entry specified by the test.
-		utxoCache.SpendEntry(test.outpoint)
+		utxoCache.spendEntry(test.outpoint)
 
 		// If the existing entry was nil or spent, continue as there is nothing
 		// else to validate.

From 72df18ec065e85fbdc2541b1b390fbdeea4e8284 Mon Sep 17 00:00:00 2001
From: Dave Collins <davec@conformal.com>
Date: Tue, 13 Sep 2022 05:24:08 -0500
Subject: [PATCH 04/19] blockchain: Remove exported utxo cache AddEntry.

This removes the exported AddEntry method from the UtxoCache type to
help ensure cache coherency by forcing all external updates to the cache
to happen via Commit which the cache fully controls.
---
 internal/blockchain/utxocache.go      | 15 ---------------
 internal/blockchain/utxocache_test.go |  4 ++--
 2 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/internal/blockchain/utxocache.go b/internal/blockchain/utxocache.go
index 69cdc9776a..650e20db2d 100644
--- a/internal/blockchain/utxocache.go
+++ b/internal/blockchain/utxocache.go
@@ -291,21 +291,6 @@ func (c *UtxoCache) addEntry(outpoint wire.OutPoint, entry *UtxoEntry) error {
 	return nil
 }
 
-// AddEntry adds the specified output to the cache.  The entry being added MUST
-// NOT be mutated by the caller after being passed to this function.
-//
-// Note that this function does not check if the entry is unspendable and
-// therefore the caller should ensure that the entry is spendable before adding
-// it to the cache.
-//
-// This function is safe for concurrent access.
-func (c *UtxoCache) AddEntry(outpoint wire.OutPoint, entry *UtxoEntry) error {
-	c.cacheLock.Lock()
-	err := c.addEntry(outpoint, entry)
-	c.cacheLock.Unlock()
-	return err
-}
-
 // spendEntry marks the specified output as spent.
 //
 // This function MUST be called with the cache lock held.
diff --git a/internal/blockchain/utxocache_test.go b/internal/blockchain/utxocache_test.go
index 796705d968..9e3ef4df0e 100644
--- a/internal/blockchain/utxocache_test.go
+++ b/internal/blockchain/utxocache_test.go
@@ -193,7 +193,7 @@ func createTestUtxoCache(t *testing.T, entries map[wire.OutPoint]*UtxoEntry) *Ut
 		// Add the entry to the cache.  The entry is cloned before being added
 		// so that any modifications that the cache makes to the entry are not
 		// reflected in the provided test entry.
-		err := utxoCache.AddEntry(outpoint, entry.Clone())
+		err := utxoCache.addEntry(outpoint, entry.Clone())
 		if err != nil {
 			t.Fatalf("unexpected error when adding entry: %v", err)
 		}
@@ -343,7 +343,7 @@ func TestAddEntry(t *testing.T) {
 		}
 
 		// Add the entry specified by the test.
-		err := utxoCache.AddEntry(test.outpoint, test.entry)
+		err := utxoCache.addEntry(test.outpoint, test.entry)
 		if err != nil {
 			t.Fatalf("%q: unexpected error when adding entry: %v", test.name,
 				err)

From 670eb37a57886035a4d961521b6addc3cd4bd503 Mon Sep 17 00:00:00 2001
From: Dave Collins <davec@conformal.com>
Date: Tue, 13 Sep 2022 05:24:09 -0500
Subject: [PATCH 05/19] blockchain: Remove unused utxo cache add entry err.

This removes the error return from the UtxoCache.addEntry method since
it does not return any errors.
---
 internal/blockchain/utxocache.go      | 10 ++--------
 internal/blockchain/utxocache_test.go | 11 ++---------
 2 files changed, 4 insertions(+), 17 deletions(-)

diff --git a/internal/blockchain/utxocache.go b/internal/blockchain/utxocache.go
index 650e20db2d..e876c4d21c 100644
--- a/internal/blockchain/utxocache.go
+++ b/internal/blockchain/utxocache.go
@@ -265,7 +265,7 @@ func (c *UtxoCache) hitRatio() float64 {
 // it to the cache.
 //
 // This function MUST be called with the cache lock held.
-func (c *UtxoCache) addEntry(outpoint wire.OutPoint, entry *UtxoEntry) error {
+func (c *UtxoCache) addEntry(outpoint wire.OutPoint, entry *UtxoEntry) {
 	// Attempt to get an existing entry from the cache.
 	cachedEntry := c.entries[outpoint]
 
@@ -287,8 +287,6 @@ func (c *UtxoCache) addEntry(outpoint wire.OutPoint, entry *UtxoEntry) error {
 		c.totalEntrySize -= cachedEntry.size()
 	}
 	c.totalEntrySize += entry.size()
-
-	return nil
 }
 
 // spendEntry marks the specified output as spent.
@@ -464,11 +462,7 @@ func (c *UtxoCache) Commit(view *UtxoViewpoint) error {
 
 		// If we passed all of the conditions above, the entry is modified or
 		// fresh, but not spent, and should be added to the cache.
-		err := c.addEntry(outpoint, entry)
-		if err != nil {
-			c.cacheLock.Unlock()
-			return err
-		}
+		c.addEntry(outpoint, entry)
 
 		// All entries that are added to the cache should be removed from the
 		// provided view.  This is an optimization to allow the cache to take
diff --git a/internal/blockchain/utxocache_test.go b/internal/blockchain/utxocache_test.go
index 9e3ef4df0e..e84e0f3551 100644
--- a/internal/blockchain/utxocache_test.go
+++ b/internal/blockchain/utxocache_test.go
@@ -193,10 +193,7 @@ func createTestUtxoCache(t *testing.T, entries map[wire.OutPoint]*UtxoEntry) *Ut
 		// Add the entry to the cache.  The entry is cloned before being added
 		// so that any modifications that the cache makes to the entry are not
 		// reflected in the provided test entry.
-		err := utxoCache.addEntry(outpoint, entry.Clone())
-		if err != nil {
-			t.Fatalf("unexpected error when adding entry: %v", err)
-		}
+		utxoCache.addEntry(outpoint, entry.Clone())
 
 		// Set the state of the cached entries based on the provided entries.
 		// This is allowed for tests to easily simulate entries in the cache
@@ -343,11 +340,7 @@ func TestAddEntry(t *testing.T) {
 		}
 
 		// Add the entry specified by the test.
-		err := utxoCache.addEntry(test.outpoint, test.entry)
-		if err != nil {
-			t.Fatalf("%q: unexpected error when adding entry: %v", test.name,
-				err)
-		}
+		utxoCache.addEntry(test.outpoint, test.entry)
 		wantTotalEntrySize += test.entry.size()
 
 		// Attempt to get the added entry from the cache.

From 3315ace867ad96338aba9dfc809223efa60a9e15 Mon Sep 17 00:00:00 2001
From: Dave Collins <davec@conformal.com>
Date: Tue, 13 Sep 2022 05:24:10 -0500
Subject: [PATCH 06/19] blockchain: Remove tip param from utxo cache init.

This removes the tip parameter from the utxo cache Initialize method
since it is unneccesary given it takes the BlockChain as a paramter
which has the tip available on it.
---
 internal/blockchain/chain.go          | 2 +-
 internal/blockchain/utxocache.go      | 5 +++--
 internal/blockchain/utxocache_test.go | 6 ++----
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/internal/blockchain/chain.go b/internal/blockchain/chain.go
index 7c69c56158..1265adc9ad 100644
--- a/internal/blockchain/chain.go
+++ b/internal/blockchain/chain.go
@@ -2212,7 +2212,7 @@ func New(ctx context.Context, config *Config) (*BlockChain, error) {
 
 	// Initialize the UTXO state.  This entails running any database migrations
 	// as necessary as well as initializing the UTXO cache.
-	if err := b.utxoCache.Initialize(ctx, &b, b.bestChain.tip()); err != nil {
+	if err := b.utxoCache.Initialize(ctx, &b); err != nil {
 		return nil, err
 	}
 
diff --git a/internal/blockchain/utxocache.go b/internal/blockchain/utxocache.go
index e876c4d21c..6c98922355 100644
--- a/internal/blockchain/utxocache.go
+++ b/internal/blockchain/utxocache.go
@@ -102,7 +102,7 @@ type UtxoCacher interface {
 	// Initialize initializes the utxo cache and underlying utxo backend.  This
 	// entails running any database migrations as well as ensuring that the utxo
 	// set is caught up to the tip of the best chain.
-	Initialize(ctx context.Context, b *BlockChain, tip *blockNode) error
+	Initialize(ctx context.Context, b *BlockChain) error
 
 	// MaybeFlush conditionally flushes the cache to the backend.  A flush can
 	// be forced by setting the force flush parameter.
@@ -668,7 +668,7 @@ func (c *UtxoCache) MaybeFlush(bestHash *chainhash.Hash, bestHeight uint32,
 // last flushed to the tip block through the cache.
 //
 // This function should only be called during initialization.
-func (c *UtxoCache) Initialize(ctx context.Context, b *BlockChain, tip *blockNode) error {
+func (c *UtxoCache) Initialize(ctx context.Context, b *BlockChain) error {
 	log.Infof("UTXO cache initializing (max size: %d MiB)...",
 		c.maxSize/1024/1024)
 
@@ -687,6 +687,7 @@ func (c *UtxoCache) Initialize(ctx context.Context, b *BlockChain, tip *blockNod
 	// If the state is nil, update the state to the tip.  This should only be
 	// the case when starting from a fresh backend or a backend that has not
 	// been run with the utxo cache yet.
+	tip := b.bestChain.Tip()
 	if state == nil {
 		state = &UtxoSetState{
 			lastFlushHeight: uint32(tip.height),
diff --git a/internal/blockchain/utxocache_test.go b/internal/blockchain/utxocache_test.go
index e84e0f3551..37ee146c0b 100644
--- a/internal/blockchain/utxocache_test.go
+++ b/internal/blockchain/utxocache_test.go
@@ -1075,8 +1075,7 @@ func TestInitialize(t *testing.T) {
 			MaxSize:      100 * 1024 * 1024, // 100 MiB
 		})
 		g.chain.utxoCache = testUtxoCache
-		err := testUtxoCache.Initialize(context.Background(), g.chain,
-			g.chain.bestChain.Tip())
+		err := testUtxoCache.Initialize(context.Background(), g.chain)
 		if err != nil {
 			t.Fatalf("error initializing test cache: %v", err)
 		}
@@ -1225,8 +1224,7 @@ func TestShutdownUtxoCache(t *testing.T) {
 	if err != nil {
 		t.Fatalf("error initializing backend info: %v", err)
 	}
-	err = testUtxoCache.Initialize(context.Background(), g.chain,
-		g.chain.bestChain.Tip())
+	err = testUtxoCache.Initialize(context.Background(), g.chain)
 	if err != nil {
 		t.Fatalf("error initializing test cache: %v", err)
 	}

From b666cac3404e79a4c127446fc3d5c0be5e260675 Mon Sep 17 00:00:00 2001
From: Dave Collins <davec@conformal.com>
Date: Tue, 13 Sep 2022 05:24:11 -0500
Subject: [PATCH 07/19] blockchain: Allow tests to override cache flushing.

This modifies the utxo cache tests to ensure the overridden MaybeFlush
method is called for all utxo cache methods as opposed to only when
accessed via the UtxoCacher interface.

Perhaps most notably, the overridden method is now called when the cache
is being initialized which allows its logic to be better tested.

It accomplishes this by adding a new closure to the UtxoCache struct
that defaults to the exported UtxoCache.MaybeAccept, updates all
internal calls to make use of it, and then overrides it when creating
the test harness.
---
 internal/blockchain/utxocache.go      | 20 ++++++++++++++++----
 internal/blockchain/utxocache_test.go |  7 +++++--
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/internal/blockchain/utxocache.go b/internal/blockchain/utxocache.go
index 6c98922355..4494496957 100644
--- a/internal/blockchain/utxocache.go
+++ b/internal/blockchain/utxocache.go
@@ -186,6 +186,12 @@ type UtxoCache struct {
 	// defaults to time.Now but an alternative function can be provided for
 	// testing purposes.
 	timeNow func() time.Time
+
+	// maybeFlushFn defines the function to use for potentially flushing the
+	// cache to the backend.  It defaults to the exported MaybeFlush method of
+	// this cache instance, but an alternative can be provided for testing
+	// purposes.
+	maybeFlushFn func(*chainhash.Hash, uint32, bool, bool) error
 }
 
 // Ensure UtxoCache implements the UtxoCacher interface.
@@ -222,7 +228,7 @@ func NewUtxoCache(config *UtxoCacheConfig) *UtxoCache {
 		p2pkhScriptLen
 	maxEntries := math.Ceil(float64(config.MaxSize) / float64(avgEntrySize))
 
-	return &UtxoCache{
+	c := &UtxoCache{
 		backend:       config.Backend,
 		flushBlockDB:  config.FlushBlockDB,
 		maxSize:       config.MaxSize,
@@ -230,6 +236,8 @@ func NewUtxoCache(config *UtxoCacheConfig) *UtxoCache {
 		lastFlushTime: time.Now(),
 		timeNow:       time.Now,
 	}
+	c.maybeFlushFn = c.MaybeFlush
+	return c
 }
 
 // totalSize returns the total size of the cache on a 64-bit platform, in bytes.
@@ -418,7 +426,7 @@ func (c *UtxoCache) FetchBackendState() (*UtxoSetState, error) {
 func (c *UtxoCache) FetchStats(bestHash *chainhash.Hash, bestHeight uint32) (*UtxoStats, error) {
 	// Force a UTXO cache flush.  This is required in order for the backend to
 	// fetch statistics on the full UTXO set.
-	err := c.MaybeFlush(bestHash, bestHeight, true, false)
+	err := c.maybeFlushFn(bestHash, bestHeight, true, false)
 	if err != nil {
 		return nil, err
 	}
@@ -806,7 +814,9 @@ func (c *UtxoCache) Initialize(ctx context.Context, b *BlockChain) error {
 		// Conditionally flush the utxo cache to the backend.  Don't force flush
 		// since many blocks may be disconnected and connected in quick
 		// succession when initializing.
-		err = c.MaybeFlush(&n.hash, uint32(n.height), false, true)
+		const forceFlush = false
+		const logFlush = true
+		err = c.maybeFlushFn(&n.hash, uint32(n.height), forceFlush, logFlush)
 		if err != nil {
 			return err
 		}
@@ -881,7 +891,9 @@ func (c *UtxoCache) Initialize(ctx context.Context, b *BlockChain) error {
 		// Conditionally flush the utxo cache to the backend.  Don't force flush
 		// since many blocks may be connected in quick succession when
 		// initializing.
-		err = c.MaybeFlush(&n.hash, uint32(n.height), false, true)
+		const forceFlush = false
+		const logFlush = true
+		err = c.maybeFlushFn(&n.hash, uint32(n.height), forceFlush, logFlush)
 		if err != nil {
 			return err
 		}
diff --git a/internal/blockchain/utxocache_test.go b/internal/blockchain/utxocache_test.go
index 37ee146c0b..b0f2fc8bd3 100644
--- a/internal/blockchain/utxocache_test.go
+++ b/internal/blockchain/utxocache_test.go
@@ -172,9 +172,12 @@ func (c *testUtxoCache) MaybeFlush(bestHash *chainhash.Hash, bestHeight uint32,
 // newTestUtxoCache returns a testUtxoCache instance using the provided
 // configuration details.
 func newTestUtxoCache(config *UtxoCacheConfig) *testUtxoCache {
-	return &testUtxoCache{
-		UtxoCache: NewUtxoCache(config),
+	utxoCache := NewUtxoCache(config)
+	c := &testUtxoCache{
+		UtxoCache: utxoCache,
 	}
+	utxoCache.maybeFlushFn = c.MaybeFlush
+	return c
 }
 
 // Ensure testUtxoCache implements the UtxoCacher interface.

From cab1a517972535388c0b1b6b6302adcc0f202b33 Mon Sep 17 00:00:00 2001
From: Dave Collins <davec@conformal.com>
Date: Tue, 13 Sep 2022 05:24:11 -0500
Subject: [PATCH 08/19] blockchain: Improve utxo cache initialize tests.

Currently, the utxo cache initialize tests don't actually test that the
initialize code itself flushes the expected data to the backend rather
they do a flush from the tests themselves using the best chain tip which
may or may not match whatever initialize actually ordinarily
conditionally flushed.

This modifies the utxo cache initialize tests force flushing internally
when the cache is being initialized accordingly to better ensure it is
flushing the expected data.
---
 internal/blockchain/common_test.go    |  6 ++++--
 internal/blockchain/utxocache_test.go | 17 ++++++++++-------
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/internal/blockchain/common_test.go b/internal/blockchain/common_test.go
index 00fbe80d9a..0fd78bce56 100644
--- a/internal/blockchain/common_test.go
+++ b/internal/blockchain/common_test.go
@@ -902,8 +902,10 @@ func (g *chaingenHarness) ExpectUtxoSetState(blockName string) {
 		lastFlushHash:   block.BlockHash(),
 	}
 	if !reflect.DeepEqual(gotState, wantState) {
-		g.t.Fatalf("mismatched utxo set state:\nwant: %+v\n got: %+v\n", wantState,
-			gotState)
+		g.t.Fatalf("mismatched utxo set state:\nwant: hash %s, height %d\n "+
+			"got: hash %s, height %d\n", wantState.lastFlushHash,
+			wantState.lastFlushHeight, gotState.lastFlushHash,
+			gotState.lastFlushHeight)
 	}
 }
 
diff --git a/internal/blockchain/utxocache_test.go b/internal/blockchain/utxocache_test.go
index b0f2fc8bd3..900f290fab 100644
--- a/internal/blockchain/utxocache_test.go
+++ b/internal/blockchain/utxocache_test.go
@@ -152,7 +152,8 @@ func entry85314() *UtxoEntry {
 // simulate various scenarios.
 type testUtxoCache struct {
 	*UtxoCache
-	disableFlush bool
+	disableFlush bool // disable flushing unconditionally (even if forced)
+	forceFlush   bool // force flushing even when not requested by caller
 }
 
 // MaybeFlush conditionally flushes the cache to the backend.  If the disable
@@ -166,6 +167,9 @@ func (c *testUtxoCache) MaybeFlush(bestHash *chainhash.Hash, bestHeight uint32,
 		return nil
 	}
 
+	// Force a flush if either the flag provided by the caller is true or the
+	// test cache overrides it.
+	forceFlush = forceFlush || c.forceFlush
 	return c.UtxoCache.MaybeFlush(bestHash, bestHeight, forceFlush, logFlush)
 }
 
@@ -1071,12 +1075,13 @@ func TestInitialize(t *testing.T) {
 	if err != nil {
 		t.Fatalf("error initializing backend info: %v", err)
 	}
-	resetTestUtxoCache := func() *testUtxoCache {
+	resetTestUtxoCache := func(forceFlush bool) *testUtxoCache {
 		testUtxoCache := newTestUtxoCache(&UtxoCacheConfig{
 			Backend:      backend,
 			FlushBlockDB: g.chain.db.Flush,
 			MaxSize:      100 * 1024 * 1024, // 100 MiB
 		})
+		testUtxoCache.forceFlush = forceFlush
 		g.chain.utxoCache = testUtxoCache
 		err := testUtxoCache.Initialize(context.Background(), g.chain)
 		if err != nil {
@@ -1101,7 +1106,7 @@ func TestInitialize(t *testing.T) {
 
 	// Replace the utxo cache in the test chain with a test utxo cache so that
 	// flushing can be toggled on and off for testing.
-	testUtxoCache := resetTestUtxoCache()
+	testUtxoCache := resetTestUtxoCache(false)
 
 	// Validate that the tip and utxo set state are currently at the genesis
 	// block.
@@ -1118,8 +1123,7 @@ func TestInitialize(t *testing.T) {
 	g.ExpectUtxoSetState("genesis")
 
 	// Reset the cache and force a flush.
-	testUtxoCache = resetTestUtxoCache()
-	forceFlush(testUtxoCache)
+	testUtxoCache = resetTestUtxoCache(true)
 
 	// Validate that the utxo cache is now caught up to the tip.
 	g.ExpectUtxoSetState(g.TipName())
@@ -1201,8 +1205,7 @@ func TestInitialize(t *testing.T) {
 	g.ExpectUtxoSetState("b1")
 
 	// Reset the cache and force a flush.
-	testUtxoCache = resetTestUtxoCache()
-	forceFlush(testUtxoCache)
+	testUtxoCache = resetTestUtxoCache(true)
 
 	// Validate that the cache recovered and is now caught up to b1a.
 	g.ExpectUtxoSetState("b1a")

From e563175e6abda1d449b3722d6e46cec33699f8a3 Mon Sep 17 00:00:00 2001
From: Dave Collins <davec@conformal.com>
Date: Tue, 13 Sep 2022 05:24:12 -0500
Subject: [PATCH 09/19] blockchain: Fix rare unclean utxo cache recovery.

This corrects an extremely hard to hit corner case involving a mix of
manual block invalidation, conditional flushing, and successive unclean
shutdowns and adds a test to ensure proper behavior.

Specifically, the recovery code from an unclean shutdown that unwinds
the cache potentially periodically flushes the state to the backend was
incorrectly using the block being disconnected as opposed to its parent
which is now the best tip.

In practice, this is exceedingly difficult to hit without intentionally
trying because it requires a mix of highly unlikely events and manual
invalidation:

- Two unclean shutdowns at precise inopportune times
- Manual invalidation of blocks by the operator such that blocks are
  only disconnected without reconnecting a side chain
- The recovery taking long enough or involving enough entries to trigger
  a conditional flush, again, at exactly the right time prior to a
  second unclean shutdown
---
 internal/blockchain/utxocache.go      |  3 +-
 internal/blockchain/utxocache_test.go | 68 +++++++++++++++++++++++++++
 2 files changed, 70 insertions(+), 1 deletion(-)

diff --git a/internal/blockchain/utxocache.go b/internal/blockchain/utxocache.go
index 4494496957..e308efb05c 100644
--- a/internal/blockchain/utxocache.go
+++ b/internal/blockchain/utxocache.go
@@ -816,7 +816,8 @@ func (c *UtxoCache) Initialize(ctx context.Context, b *BlockChain) error {
 		// succession when initializing.
 		const forceFlush = false
 		const logFlush = true
-		err = c.maybeFlushFn(&n.hash, uint32(n.height), forceFlush, logFlush)
+		err = c.maybeFlushFn(&n.parent.hash, uint32(n.parent.height),
+			forceFlush, logFlush)
 		if err != nil {
 			return err
 		}
diff --git a/internal/blockchain/utxocache_test.go b/internal/blockchain/utxocache_test.go
index 900f290fab..eb5da582dd 100644
--- a/internal/blockchain/utxocache_test.go
+++ b/internal/blockchain/utxocache_test.go
@@ -14,6 +14,7 @@ import (
 	"github.com/decred/dcrd/chaincfg/chainhash"
 	"github.com/decred/dcrd/chaincfg/v3"
 	"github.com/decred/dcrd/database/v3"
+	"github.com/decred/dcrd/dcrutil/v4"
 	"github.com/decred/dcrd/wire"
 )
 
@@ -1136,10 +1137,12 @@ func TestInitialize(t *testing.T) {
 
 	outs := g.OldestCoinbaseOuts()
 	g.NextBlock("b0", &outs[0], outs[1:])
+	g.SaveTipCoinbaseOuts()
 	g.AcceptTipBlock()
 
 	outs = g.OldestCoinbaseOuts()
 	b1 := g.NextBlock("b1", &outs[0], outs[1:])
+	g.SaveTipCoinbaseOuts()
 	g.AcceptTipBlock()
 
 	// Force a cache flush and validate that the cache is caught up to block b1.
@@ -1209,6 +1212,71 @@ func TestInitialize(t *testing.T) {
 
 	// Validate that the cache recovered and is now caught up to b1a.
 	g.ExpectUtxoSetState("b1a")
+
+	// -------------------------------------------------------------------------
+	// Simulate an unclean shutdown such that the utxocache was last flushed at
+	// the tip block which is later invalidated and ensure the cache and backend
+	// recover back to the parent block as expected.
+	//
+	// This is accomplished by creating a new block that becomes the new best
+	// chain tip, disabling flushing, and invalidating that new block to ensure
+	// that the flushed utxo cache state is the invalidated block.  Then the
+	// cache is reset while forcing flushing during initialization.
+	//
+	//   ... -> b0 -> b1
+	//            |      \-> b2bad
+	//            \-> b1a    -----
+	//                       ^ (marked invalid with no flush to cache backend)
+	// -------------------------------------------------------------------------
+
+	outs = g.OldestCoinbaseOuts()
+	g.SetTip("b1")
+	b2bad := g.NextBlock("b2bad", &outs[0], outs[1:])
+	g.AcceptTipBlock()
+	forceFlush(testUtxoCache)
+	g.ExpectUtxoSetState("b2bad")
+
+	// Disable flushing to ensure the block that is about to be invalidated is
+	// not flushed to the backend.
+	//
+	// Ordinarily, the spend journal entry for disconnected blocks is not
+	// removed when there is an error in flushing, however, since this bypasses
+	// the error, the entry needs to be saved and restored after the block is
+	// invalidated.
+	testUtxoCache.disableFlush = true
+	var b2badStxos []spentTxOut
+	err = g.chain.db.View(func(dbTx database.Tx) error {
+		prevHash := b2bad.Header.PrevBlock
+		isTrsyEnabled, err := g.chain.IsTreasuryAgendaActive(&prevHash)
+		if err != nil {
+			return err
+		}
+		block := dcrutil.NewBlock(b2bad)
+		b2badStxos, err = dbFetchSpendJournalEntry(dbTx, block, isTrsyEnabled)
+		return err
+	})
+	if err != nil {
+		t.Fatalf("unexpected error getting spend journal entry: %v", err)
+	}
+
+	// Invalidate the previously connected block so that it is disconnected and
+	// ensure the flushed utxo cache state is still at the invalidated block.
+	g.InvalidateBlockAndExpectTip("b2bad", nil, "b1")
+	g.ExpectUtxoSetState("b2bad")
+
+	// Restore the spend journal entry for the invalidated block per the above.
+	err = g.chain.db.Update(func(dbTx database.Tx) error {
+		b2badHash := b2bad.BlockHash()
+		return dbPutSpendJournalEntry(dbTx, &b2badHash, b2badStxos)
+	})
+	if err != nil {
+		t.Fatalf("unexpected error restoring spend journal entry: %v", err)
+	}
+
+	// Reset the cache while forcing flushing during the initialization and
+	// ensure the cache and backend recover back to b1 as expected.
+	resetTestUtxoCache(true)
+	g.ExpectUtxoSetState("b1")
 }
 
 // TestShutdownUtxoCache validates that a cache flush is forced when shutting

From 8b19171b7a73b5ce2e5359340a9340dabc650ea2 Mon Sep 17 00:00:00 2001
From: Dave Collins <davec@conformal.com>
Date: Tue, 13 Sep 2022 05:24:13 -0500
Subject: [PATCH 10/19] blockchain: Don't fetch trsy{base,spend} inputs.

This modifies the code that deal with fetching input utxos to avoid
needlessly looking up inputs for treasurybase and treasury spend
transactions since they are both similar to coinbase transactions in
that they do not reference any real previous outputs and therefore are
guaranteed to result in a cache miss.
---
 internal/blockchain/utxoviewpoint.go | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/internal/blockchain/utxoviewpoint.go b/internal/blockchain/utxoviewpoint.go
index 25d35bcb7d..e9ee6debd7 100644
--- a/internal/blockchain/utxoviewpoint.go
+++ b/internal/blockchain/utxoviewpoint.go
@@ -748,9 +748,20 @@ func (view *UtxoViewpoint) fetchInputUtxos(block *dcrutil.Block,
 	// stake tree are not allowed to access outputs of transactions earlier in
 	// the block.  This applies to both transactions earlier in the stake tree
 	// as well as those in the regular tree.
-	for _, stx := range block.STransactions() {
-		isVote := stake.IsSSGen(stx.MsgTx())
-		for txInIdx, txIn := range stx.MsgTx().TxIn {
+	for txIdx, stx := range block.STransactions() {
+		// Ignore treasurybases and treasury spends since they have no inputs.
+		msgTx := stx.MsgTx()
+		shouldBeTreasuryBase := isTreasuryEnabled && txIdx == 0
+		if shouldBeTreasuryBase && stake.IsTreasuryBase(msgTx) {
+			continue
+		}
+		isTreasurySpend := isTreasuryEnabled && stake.IsTSpend(msgTx)
+		if isTreasurySpend {
+			continue
+		}
+
+		isVote := stake.IsSSGen(msgTx)
+		for txInIdx, txIn := range msgTx.TxIn {
 			// Ignore stakebase since it has no input.
 			if isVote && txInIdx == 0 {
 				continue
@@ -866,8 +877,13 @@ func (b *BlockChain) FetchUtxoView(tx *dcrutil.Tx, includeRegularTxns bool) (*Ut
 		outpoint.Index = uint32(txOutIdx)
 		filteredSet.add(view, &outpoint)
 	}
-	if !standalone.IsCoinBaseTx(msgTx, isTreasuryEnabled) {
-		isVote := stake.IsSSGen(msgTx)
+	// Ignore coinbases, treasurybases, and treasury spends since none of them
+	// have any inputs.
+	isCoinBase := standalone.IsCoinBaseTx(msgTx, isTreasuryEnabled)
+	isTreasuryBase := txType == stake.TxTypeTreasuryBase
+	isTreasurySpend := txType == stake.TxTypeTSpend
+	if !isCoinBase && !isTreasuryBase && !isTreasurySpend {
+		isVote := txType == stake.TxTypeSSGen
 		for txInIdx, txIn := range msgTx.TxIn {
 			// Ignore stakebase since it has no input.
 			if isVote && txInIdx == 0 {

From d63df89c288d8e6620cd34fca43fca723e5ca984 Mon Sep 17 00:00:00 2001
From: Dave Collins <davec@conformal.com>
Date: Tue, 13 Sep 2022 05:24:14 -0500
Subject: [PATCH 11/19] blockchain: Don't add treasurybase utxos.

This updates the code when connecting transactions in a utxo view to
check for treasurybases prior to the coinbase since a treasurybase is
also identified as a coinbase, but unlike coinbases, treasurybase
outputs are never spendable and thus should not be added to the utxo
view.
---
 internal/blockchain/utxoviewpoint.go | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/internal/blockchain/utxoviewpoint.go b/internal/blockchain/utxoviewpoint.go
index e9ee6debd7..6dee5770f0 100644
--- a/internal/blockchain/utxoviewpoint.go
+++ b/internal/blockchain/utxoviewpoint.go
@@ -224,16 +224,20 @@ func (view *UtxoViewpoint) PriorityInput(prevOut *wire.OutPoint) (int64, int64,
 func (view *UtxoViewpoint) connectTransaction(tx *dcrutil.Tx, blockHeight int64,
 	blockIndex uint32, stxos *[]spentTxOut, isTreasuryEnabled bool) error {
 
-	// Coinbase transactions don't have any inputs to spend.
+	// Treasurybase transactions don't have any inputs to spend.
+	//
+	// NOTE: This check MUST come before the coinbase check because a
+	// treasurybase is identified as a coinbase as of the time this comment is
+	// being written.
 	msgTx := tx.MsgTx()
-	if standalone.IsCoinBaseTx(msgTx, isTreasuryEnabled) {
-		// Add the transaction's outputs as available utxos.
-		view.AddTxOuts(tx, blockHeight, blockIndex, isTreasuryEnabled)
+	if isTreasuryEnabled && standalone.IsTreasuryBase(msgTx) {
 		return nil
 	}
 
-	// Treasurybase transactions don't have any inputs to spend.
-	if isTreasuryEnabled && standalone.IsTreasuryBase(msgTx) {
+	// Coinbase transactions don't have any inputs to spend.
+	if standalone.IsCoinBaseTx(msgTx, isTreasuryEnabled) {
+		// Add the transaction's outputs as available utxos.
+		view.AddTxOuts(tx, blockHeight, blockIndex, isTreasuryEnabled)
 		return nil
 	}
 

From a6e3c928456fd69943d4a6827b022dd9cc3d7453 Mon Sep 17 00:00:00 2001
From: Dave Collins <davec@conformal.com>
Date: Tue, 13 Sep 2022 05:24:15 -0500
Subject: [PATCH 12/19] blockchain: Consolidate utxo cache test entries.

This simplifies the various utxo cache tests and improves their
legibility by consolidating the utxo test entry state creation logic and
updating the tests to make use of it accordingly.

In particular, it introduces a struct to house entries with the various
states along with a method to create them.
---
 internal/blockchain/utxocache_test.go | 208 +++++++++++++-------------
 1 file changed, 103 insertions(+), 105 deletions(-)

diff --git a/internal/blockchain/utxocache_test.go b/internal/blockchain/utxocache_test.go
index eb5da582dd..a0b40cb904 100644
--- a/internal/blockchain/utxocache_test.go
+++ b/internal/blockchain/utxocache_test.go
@@ -148,6 +148,51 @@ func entry85314() *UtxoEntry {
 	}
 }
 
+// testEntryStates houses a test utxo entry along with various spent, modified,
+// and fresh states for conenient use throughout the tests.
+type testEntryStates struct {
+	unmodified           *UtxoEntry
+	unmodifiedSpent      *UtxoEntry
+	unmodifiedFresh      *UtxoEntry
+	unmodifiedSpentFresh *UtxoEntry
+	modified             *UtxoEntry
+	modifiedSpent        *UtxoEntry
+	modifiedFresh        *UtxoEntry
+	modifiedSpentFresh   *UtxoEntry
+}
+
+// makeEntryStates creates combinations of the passed test utxo entry in various
+// states for convenient use in the tests below.
+func makeEntryStates(entry *UtxoEntry) testEntryStates {
+	unmodified := entry.Clone()
+	unmodifiedSpent := unmodified.Clone()
+	unmodifiedSpent.state |= utxoStateSpent
+	unmodifiedFresh := unmodified.Clone()
+	unmodifiedFresh.state |= utxoStateFresh
+	unmodifiedSpentFresh := unmodifiedSpent.Clone()
+	unmodifiedSpentFresh.state |= utxoStateFresh
+
+	modified := entry.Clone()
+	modified.state |= utxoStateModified
+	modifiedSpent := modified.Clone()
+	modifiedSpent.state |= utxoStateSpent
+	modifiedFresh := modified.Clone()
+	modifiedFresh.state |= utxoStateFresh
+	modifiedSpentFresh := modifiedSpent.Clone()
+	modifiedSpentFresh.state |= utxoStateFresh
+
+	return testEntryStates{
+		unmodified:           unmodified,
+		unmodifiedSpent:      unmodifiedSpent,
+		unmodifiedFresh:      unmodifiedFresh,
+		unmodifiedSpentFresh: unmodifiedSpentFresh,
+		modified:             modified,
+		modifiedSpent:        modifiedSpent,
+		modifiedFresh:        modifiedFresh,
+		modifiedSpentFresh:   modifiedSpentFresh,
+	}
+}
+
 // testUtxoCache provides a mock utxo cache by implementing the UtxoCacher
 // interface.  It allows for toggling flushing on and off to more easily
 // simulate various scenarios.
@@ -305,13 +350,8 @@ func TestAddEntry(t *testing.T) {
 	t.Parallel()
 
 	// Create test entries to be used throughout the tests.
-	outpoint := outpoint299()
-	entry := entry299()
-	entryModified := entry.Clone()
-	entryModified.amount++
-	entryModified.state |= utxoStateModified
-	entryFresh := entry.Clone()
-	entryFresh.state |= utxoStateModified | utxoStateFresh
+	outpoint, entry := outpoint299(), makeEntryStates(entry299())
+	entry.modified.amount++
 
 	tests := []struct {
 		name            string
@@ -322,16 +362,16 @@ func TestAddEntry(t *testing.T) {
 	}{{
 		name:      "add an entry that does not already exist in the cache",
 		outpoint:  outpoint,
-		entry:     entry,
-		wantEntry: entryFresh,
+		entry:     entry.unmodified,
+		wantEntry: entry.modifiedFresh,
 	}, {
 		name: "add an entry that overwrites an existing entry",
 		existingEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint: entry,
+			outpoint: entry.unmodified,
 		},
 		outpoint:  outpoint,
-		entry:     entryModified,
-		wantEntry: entryModified,
+		entry:     entry.modified,
+		wantEntry: entry.modified,
 	}}
 
 	for _, test := range tests {
@@ -386,12 +426,7 @@ func TestSpendEntry(t *testing.T) {
 	t.Parallel()
 
 	// Create test entries to be used throughout the tests.
-	outpoint := outpoint299()
-	entry := entry299()
-	entryFresh := entry.Clone()
-	entryFresh.state |= utxoStateModified | utxoStateFresh
-	entrySpent := entry.Clone()
-	entrySpent.Spend()
+	outpoint, entry := outpoint299(), makeEntryStates(entry299())
 
 	tests := []struct {
 		name            string
@@ -401,28 +436,28 @@ func TestSpendEntry(t *testing.T) {
 	}{{
 		name:     "spend an entry that does not exist in the cache",
 		outpoint: outpoint,
-		entry:    entry,
+		entry:    entry.unmodified,
 	}, {
 		name: "spend an entry that exists in the cache but is already spent",
 		existingEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint: entrySpent,
+			outpoint: entry.unmodifiedSpent,
 		},
 		outpoint: outpoint,
-		entry:    entrySpent,
+		entry:    entry.unmodifiedSpent,
 	}, {
 		name: "spend an entry that exists in the cache and is fresh",
 		existingEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint: entryFresh,
+			outpoint: entry.modifiedFresh,
 		},
 		outpoint: outpoint,
-		entry:    entryFresh,
+		entry:    entry.modifiedFresh,
 	}, {
 		name: "spend an entry that exists in the cache and is not fresh",
 		existingEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint: entry,
+			outpoint: entry.unmodified,
 		},
 		outpoint: outpoint,
-		entry:    entry,
+		entry:    entry.unmodified,
 	}}
 
 	for _, test := range tests {
@@ -483,10 +518,7 @@ func TestFetchEntry(t *testing.T) {
 	backend := createTestUtxoBackend(t)
 
 	// Create test entries to be used throughout the tests.
-	outpoint := outpoint299()
-	entry := entry299()
-	entryModified := entry.Clone()
-	entryModified.state |= utxoStateModified
+	outpoint, entry := outpoint299(), makeEntryStates(entry299())
 
 	tests := []struct {
 		name           string
@@ -501,18 +533,18 @@ func TestFetchEntry(t *testing.T) {
 	}, {
 		name: "entry is in the cache",
 		cachedEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint: entry,
+			outpoint: entry.unmodified,
 		},
 		outpoint:  outpoint,
 		cacheHit:  true,
-		wantEntry: entry,
+		wantEntry: entry.unmodified,
 	}, {
 		name: "entry is not in the cache but is in the backend",
 		backendEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint: entryModified,
+			outpoint: entry.modified,
 		},
 		outpoint:  outpoint,
-		wantEntry: entry,
+		wantEntry: entry.unmodified,
 	}}
 
 	for _, test := range tests {
@@ -529,15 +561,15 @@ func TestFetchEntry(t *testing.T) {
 		}
 
 		// Attempt to fetch the entry for the outpoint specified by the test.
-		entry, err = utxoCache.FetchEntry(test.outpoint)
+		gotEntry, err := utxoCache.FetchEntry(test.outpoint)
 		if err != nil {
 			t.Fatalf("%q: unexpected error fetching entry: %v", test.name, err)
 		}
 
 		// Ensure that the fetched entry matches the expected entry.
-		if !reflect.DeepEqual(entry, test.wantEntry) {
+		if !reflect.DeepEqual(gotEntry, test.wantEntry) {
 			t.Fatalf("%q: mismatched entry:\nwant: %+v\n got: %+v\n", test.name,
-				test.wantEntry, entry)
+				test.wantEntry, gotEntry)
 		}
 
 		// Ensure that the entry is now cached.
@@ -578,12 +610,8 @@ func TestFetchEntries(t *testing.T) {
 
 	// Create test entries to be used throughout the tests.
 	outpoint299 := outpoint299()
-	outpoint1100 := outpoint1100()
-	entry1100 := entry1100()
-	outpoint1200 := outpoint1200()
-	entry1200 := entry1200()
-	entry1200Modified := entry1200.Clone()
-	entry1200Modified.state |= utxoStateModified
+	outpoint1100, entry1100 := outpoint1100(), makeEntryStates(entry1100())
+	outpoint1200, entry1200 := outpoint1200(), makeEntryStates(entry1200())
 
 	tests := []struct {
 		name           string
@@ -594,10 +622,10 @@ func TestFetchEntries(t *testing.T) {
 	}{{
 		name: "entries are fetched from the cache and the backend",
 		cachedEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint1100: entry1100,
+			outpoint1100: entry1100.unmodified,
 		},
 		backendEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint1200: entry1200Modified,
+			outpoint1200: entry1200.modified,
 		},
 		filteredSet: ViewFilteredSet{
 			outpoint299:  struct{}{},
@@ -606,8 +634,8 @@ func TestFetchEntries(t *testing.T) {
 		},
 		wantEntries: map[wire.OutPoint]*UtxoEntry{
 			outpoint299:  nil,
-			outpoint1100: entry1100,
-			outpoint1200: entry1200,
+			outpoint1100: entry1100.unmodified,
+			outpoint1200: entry1200.unmodified,
 		},
 	}}
 
@@ -646,15 +674,9 @@ func TestCommit(t *testing.T) {
 
 	// Create test entries to be used throughout the tests.
 	outpoint299 := outpoint299()
-	outpoint1100 := outpoint1100()
-	entry1100Unmodified := entry1100()
-	outpoint1200 := outpoint1200()
-	entry1200 := entry1200()
-	entry1200Spent := entry1200.Clone()
-	entry1200Spent.Spend()
-	outpoint85314 := outpoint85314()
-	entry85314Modified := entry85314()
-	entry85314Modified.state |= utxoStateModified
+	outpoint1100, entry1100 := outpoint1100(), makeEntryStates(entry1100())
+	outpoint1200, entry1200 := outpoint1200(), makeEntryStates(entry1200())
+	outpoint85314, entry85314 := outpoint85314(), makeEntryStates(entry85314())
 
 	tests := []struct {
 		name              string
@@ -666,12 +688,12 @@ func TestCommit(t *testing.T) {
 		name: "view contains nil, unmodified, spent, and modified entries",
 		viewEntries: map[wire.OutPoint]*UtxoEntry{
 			outpoint299:   nil,
-			outpoint1100:  entry1100Unmodified,
-			outpoint1200:  entry1200Spent,
-			outpoint85314: entry85314Modified,
+			outpoint1100:  entry1100.unmodified,
+			outpoint1200:  entry1200.modifiedSpent,
+			outpoint85314: entry85314.modified,
 		},
 		cachedEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint1200: entry1200,
+			outpoint1200: entry1200.unmodified,
 		},
 		// outpoint299 is removed from the view since the entry is nil.
 		// entry1100Unmodified remains in the view since it is unmodified.
@@ -679,13 +701,13 @@ func TestCommit(t *testing.T) {
 		// entry85314Modified is removed from the view since it is modified and
 		// added to the cache.
 		wantViewEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint1100: entry1100Unmodified,
+			outpoint1100: entry1100.unmodified,
 		},
 		// entry1200Spent remains in the cache but is now spent.
 		// entry85314Modified is added to the cache.
 		wantCachedEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint1200:  entry1200Spent,
-			outpoint85314: entry85314Modified,
+			outpoint1200:  entry1200.modifiedSpent,
+			outpoint85314: entry85314.modified,
 		},
 	}}
 
@@ -855,34 +877,10 @@ func TestMaybeFlush(t *testing.T) {
 	block2000Hash := mustParseHash("0000000000000c8a886e3f7c32b1bb08422066dcf" +
 		"d008de596471f11a5aff475")
 
-	// entry299Fresh is from block height 299 and is modified and fresh.
-	outpoint299 := outpoint299()
-	entry299Fresh := entry299()
-	entry299Fresh.state |= utxoStateModified | utxoStateFresh
-
-	// entry299Unmodified is from block height 299 and is unmodified.
-	entry299Unmodified := entry299()
-
-	// entry1100Spent is from block height 1100 and is modified and spent.
-	outpoint1100 := outpoint1100()
-	entry1100Spent := entry1100()
-	entry1100Spent.Spend()
-
-	// entry1100Modified is from block height 1100 and is modified and unspent.
-	entry1100Modified := entry1100()
-	entry1100Modified.state |= utxoStateModified
-
-	// entry1100Unmodified is from block height 1100 and is unspent and
-	// unmodified.
-	entry1100Unmodified := entry1100()
-
-	// entry1200Fresh is from block height 1200 and is modified and fresh.
-	outpoint1200 := outpoint1200()
-	entry1200Fresh := entry1200()
-	entry1200Fresh.state |= utxoStateModified | utxoStateFresh
-
-	// entry1200Unmodified is from block height 1200 and is unmodified.
-	entry1200Unmodified := entry1200()
+	// Create test entries to be used throughout the tests.
+	outpoint299, entry299 := outpoint299(), makeEntryStates(entry299())
+	outpoint1100, entry1100 := outpoint1100(), makeEntryStates(entry1100())
+	outpoint1200, entry1200 := outpoint1200(), makeEntryStates(entry1200())
 
 	tests := []struct {
 		name                     string
@@ -907,22 +905,22 @@ func TestMaybeFlush(t *testing.T) {
 		bestHash:           block2000Hash,
 		bestHeight:         2000,
 		cachedEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint299:  entry299Fresh,
-			outpoint1100: entry1100Spent,
-			outpoint1200: entry1200Fresh,
+			outpoint299:  entry299.modifiedFresh,
+			outpoint1100: entry1100.modifiedSpent,
+			outpoint1200: entry1200.modifiedFresh,
 		},
 		backendEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint1100: entry1100Modified,
+			outpoint1100: entry1100.modified,
 		},
 		// The cache should remain unchanged since a flush is not required.
 		wantCachedEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint299:  entry299Fresh,
-			outpoint1100: entry1100Spent,
-			outpoint1200: entry1200Fresh,
+			outpoint299:  entry299.modifiedFresh,
+			outpoint1100: entry1100.modifiedSpent,
+			outpoint1200: entry1200.modifiedFresh,
 		},
 		// The backend should remain unchanged since a flush is not required.
 		wantBackendEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint1100: entry1100Unmodified,
+			outpoint1100: entry1100.unmodified,
 		},
 		wantLastEvictionHeight:   0,
 		wantLastFlushHash:        block1000Hash,
@@ -935,27 +933,27 @@ func TestMaybeFlush(t *testing.T) {
 		bestHash:           block2000Hash,
 		bestHeight:         2000,
 		cachedEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint299:  entry299Fresh,
-			outpoint1100: entry1100Spent,
-			outpoint1200: entry1200Fresh,
+			outpoint299:  entry299.modifiedFresh,
+			outpoint1100: entry1100.modifiedSpent,
+			outpoint1200: entry1200.modifiedFresh,
 		},
 		backendEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint1100: entry1100Modified,
+			outpoint1100: entry1100.modified,
 		},
 		// entry299Fresh should be evicted from the cache due to its height.
 		// entry1100Spent should be evicted since it is spent.
 		// entry1200Fresh should remain in the cache but should now be
 		// unmodified.
 		wantCachedEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint1200: entry1200Unmodified,
+			outpoint1200: entry1200.unmodified,
 		},
 		// entry299Unmodified should be added to the backend during the flush.
 		// entry1100Unmodified should be removed from the backend since it now
 		// spent.
 		// entry1200Unmodified should be added to the backend during the flush.
 		wantBackendEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint299:  entry299Unmodified,
-			outpoint1200: entry1200Unmodified,
+			outpoint299:  entry299.unmodified,
+			outpoint1200: entry1200.unmodified,
 		},
 		wantLastEvictionHeight:   300,
 		wantLastFlushHash:        block2000Hash,

From a1f71af8f03ba6147116b62ffb91f0279db62f7b Mon Sep 17 00:00:00 2001
From: Dave Collins <davec@conformal.com>
Date: Tue, 13 Sep 2022 05:24:15 -0500
Subject: [PATCH 13/19] blockchain: Rework utxo cache spend entry tests.

This reworks the utxo cache spend entry tests to simplify them and make
them more flexible by moving the primary logic for all of the expected
results into the test definitions themselves as oppossed to special
casing depending on flags in the test body and also adds an additional
test case.

It also updates the test cache creation method to allow nil entries in
the initial cache.
---
 internal/blockchain/utxocache_test.go | 107 +++++++++++---------------
 1 file changed, 46 insertions(+), 61 deletions(-)

diff --git a/internal/blockchain/utxocache_test.go b/internal/blockchain/utxocache_test.go
index a0b40cb904..53b17a992a 100644
--- a/internal/blockchain/utxocache_test.go
+++ b/internal/blockchain/utxocache_test.go
@@ -246,7 +246,11 @@ func createTestUtxoCache(t *testing.T, entries map[wire.OutPoint]*UtxoEntry) *Ut
 		// Add the entry to the cache.  The entry is cloned before being added
 		// so that any modifications that the cache makes to the entry are not
 		// reflected in the provided test entry.
-		utxoCache.addEntry(outpoint, entry.Clone())
+		if entry != nil {
+			utxoCache.addEntry(outpoint, entry.Clone())
+		} else {
+			utxoCache.entries[outpoint] = nil
+		}
 
 		// Set the state of the cached entries based on the provided entries.
 		// This is allowed for tests to easily simulate entries in the cache
@@ -428,83 +432,64 @@ func TestSpendEntry(t *testing.T) {
 	// Create test entries to be used throughout the tests.
 	outpoint, entry := outpoint299(), makeEntryStates(entry299())
 
+	type entriesMap map[wire.OutPoint]*UtxoEntry
 	tests := []struct {
-		name            string
-		existingEntries map[wire.OutPoint]*UtxoEntry
-		outpoint        wire.OutPoint
-		entry           *UtxoEntry
+		name           string        // test description
+		cachedEntries  entriesMap    // existing cache entries
+		backendEntries entriesMap    // existing backend entries
+		outpoint       wire.OutPoint // outpoint for the entry to spend
+		wantEntry      *UtxoEntry    // expected entry in cache after spend
+		wantCacheSize  uint64        // expected size of the cache after spend
 	}{{
-		name:     "spend an entry that does not exist in the cache",
-		outpoint: outpoint,
-		entry:    entry.unmodified,
+		name:          "spending nil (aka pruned) cache entry is a noop",
+		cachedEntries: entriesMap{outpoint: nil},
+		outpoint:      outpoint,
+		wantEntry:     nil,
+		wantCacheSize: 0,
 	}, {
-		name: "spend an entry that exists in the cache but is already spent",
-		existingEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint: entry.unmodifiedSpent,
-		},
-		outpoint: outpoint,
-		entry:    entry.unmodifiedSpent,
+		name:          "spending entry not in cache is a noop",
+		outpoint:      outpoint,
+		wantEntry:     nil,
+		wantCacheSize: 0,
 	}, {
-		name: "spend an entry that exists in the cache and is fresh",
-		existingEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint: entry.modifiedFresh,
-		},
-		outpoint: outpoint,
-		entry:    entry.modifiedFresh,
+		name:          "spending cache entry already marked spent is a noop",
+		cachedEntries: entriesMap{outpoint: entry.modifiedSpent},
+		outpoint:      outpoint,
+		wantEntry:     entry.modifiedSpent,
+		wantCacheSize: entry.modifiedSpent.size(),
 	}, {
-		name: "spend an entry that exists in the cache and is not fresh",
-		existingEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint: entry.unmodified,
-		},
-		outpoint: outpoint,
-		entry:    entry.unmodified,
+		name:          "spending cache entry marked fresh makes it nil",
+		cachedEntries: entriesMap{outpoint: entry.modifiedFresh},
+		outpoint:      outpoint,
+		wantEntry:     nil,
+		wantCacheSize: 0,
+	}, {
+		name:          "spending cache entry not marked fresh makes it spent",
+		cachedEntries: entriesMap{outpoint: entry.modified},
+		outpoint:      outpoint,
+		wantEntry:     entry.modifiedSpent,
+		wantCacheSize: entry.modifiedSpent.size(),
 	}}
 
 	for _, test := range tests {
 		// Create a utxo cache with the existing entries specified by the test.
-		utxoCache := createTestUtxoCache(t, test.existingEntries)
-		wantTotalEntrySize := utxoCache.totalEntrySize
-
-		// Attempt to get an existing entry from the cache.
-		entry := utxoCache.entries[test.outpoint]
-		var entryAlreadySpent bool
-		if entry != nil {
-			entryAlreadySpent = entry.IsSpent()
-		}
+		utxoCache := createTestUtxoCache(t, test.cachedEntries)
 
 		// Spend the entry specified by the test.
 		utxoCache.spendEntry(test.outpoint)
 
-		// If the existing entry was nil or spent, continue as there is nothing
-		// else to validate.
-		if entry == nil || entryAlreadySpent {
-			continue
-		}
-
-		// If the entry is fresh, validate that it was removed from the cache
-		// when spent.
-		if entry.isFresh() {
-			wantTotalEntrySize -= test.entry.size()
-			if utxoCache.entries[test.outpoint] != nil {
-				t.Fatalf("%q: entry for outpoint %v was not removed from the "+
-					"cache", test.name, test.outpoint)
-			}
+		// Get the entry associated with the output from the cache and ensure it
+		// matches the expected one.
+		gotEntry := utxoCache.entries[test.outpoint]
+		if !reflect.DeepEqual(gotEntry, test.wantEntry) {
+			t.Fatalf("%q: unexpected entry after spend -- got: %+v, want %+v",
+				test.name, gotEntry, test.wantEntry)
 		}
 
 		// Validate that the total entry size was updated as expected.
-		if utxoCache.totalEntrySize != wantTotalEntrySize {
+		if utxoCache.totalEntrySize != test.wantCacheSize {
 			t.Fatalf("%q: unexpected total entry size -- got %v, want %v",
-				test.name, utxoCache.totalEntrySize, wantTotalEntrySize)
-		}
-
-		// If entry is not fresh, validate that it still exists in the cache and
-		// is now marked as spent.
-		if !entry.isFresh() {
-			cachedEntry := utxoCache.entries[test.outpoint]
-			if cachedEntry == nil || !cachedEntry.IsSpent() {
-				t.Fatalf("%q: expected entry for outpoint %v to exist in the "+
-					"cache and be marked spent", test.name, test.outpoint)
-			}
+				test.name, utxoCache.totalEntrySize, test.wantCacheSize)
 		}
 	}
 }

From cd171b90aa7782d3bba193b4bade7a2fd95108c1 Mon Sep 17 00:00:00 2001
From: Dave Collins <davec@conformal.com>
Date: Tue, 13 Sep 2022 05:24:16 -0500
Subject: [PATCH 14/19] blockchain: Rework utxo cache commit tests.

This reworks the utxo cache commit tests to simplify them a bit, make
them more flexible, and test a lot more conditions.  It accomplishes
this by introducing an expected error so the tests themselves can
specify the expected result as opposed to assuming it should be nil.  It
also clones all of the view entries to ensure none of the shared entries
are modified when the cache takes ownership of them during the tests.

Finally, it improves the robustness of Commit by correcting the logic
regarding unmodified spent and unspent fresh view entries discovered
while adding the tests since it was technically incorrect even though
the relevant cases are never hit in practice in the current
implementation.  This helps defend against code changes that could
easily inadvertently violate the non-obvious, and heretofore
undocumented, assumptions that avoided hitting the incorrect behavior.
---
 internal/blockchain/utxocache.go      |   8 +-
 internal/blockchain/utxocache_test.go | 216 +++++++++++++++++++++++---
 2 files changed, 195 insertions(+), 29 deletions(-)

diff --git a/internal/blockchain/utxocache.go b/internal/blockchain/utxocache.go
index e308efb05c..e1195a55e6 100644
--- a/internal/blockchain/utxocache.go
+++ b/internal/blockchain/utxocache.go
@@ -451,15 +451,15 @@ func (c *UtxoCache) Commit(view *UtxoViewpoint) error {
 			continue
 		}
 
-		// If the entry is not modified and not fresh, continue as there is
-		// nothing to do.
-		if !entry.isModified() && !entry.isFresh() {
+		// There is nothing to update in the cache nor the view when the view
+		// entry was not modified.
+		if !entry.isModified() {
 			continue
 		}
 
 		// If the entry is modified and spent, mark it as spent in the cache and
 		// then delete it from the view.
-		if entry.isModified() && entry.IsSpent() {
+		if entry.IsSpent() {
 			// Mark the entry as spent in the cache.
 			c.spendEntry(outpoint)
 
diff --git a/internal/blockchain/utxocache_test.go b/internal/blockchain/utxocache_test.go
index 53b17a992a..f2eed94992 100644
--- a/internal/blockchain/utxocache_test.go
+++ b/internal/blockchain/utxocache_test.go
@@ -6,6 +6,7 @@ package blockchain
 
 import (
 	"context"
+	"errors"
 	"reflect"
 	"testing"
 	"time"
@@ -658,40 +659,196 @@ func TestCommit(t *testing.T) {
 	t.Parallel()
 
 	// Create test entries to be used throughout the tests.
-	outpoint299 := outpoint299()
+	outpoint299, entry299 := outpoint299(), makeEntryStates(entry299())
 	outpoint1100, entry1100 := outpoint1100(), makeEntryStates(entry1100())
 	outpoint1200, entry1200 := outpoint1200(), makeEntryStates(entry1200())
 	outpoint85314, entry85314 := outpoint85314(), makeEntryStates(entry85314())
 
 	tests := []struct {
-		name              string
-		viewEntries       map[wire.OutPoint]*UtxoEntry
-		cachedEntries     map[wire.OutPoint]*UtxoEntry
-		wantViewEntries   map[wire.OutPoint]*UtxoEntry
-		wantCachedEntries map[wire.OutPoint]*UtxoEntry
+		name              string                       // test description
+		viewEntries       map[wire.OutPoint]*UtxoEntry // view to commit
+		cachedEntries     map[wire.OutPoint]*UtxoEntry // existing cache entries
+		wantViewEntries   map[wire.OutPoint]*UtxoEntry // expected committed view
+		wantCachedEntries map[wire.OutPoint]*UtxoEntry // expected committed cache
+		err               error                        // expected error
 	}{{
-		name: "view contains nil, unmodified, spent, and modified entries",
+		name: "modified spent view entry w/ existing spent cache entry is noop",
+		viewEntries: map[wire.OutPoint]*UtxoEntry{
+			outpoint1200: entry1200.modifiedSpent,
+		},
+		cachedEntries: map[wire.OutPoint]*UtxoEntry{
+			outpoint1200: entry1200.modifiedSpent,
+		},
+		wantViewEntries: map[wire.OutPoint]*UtxoEntry{},
+		wantCachedEntries: map[wire.OutPoint]*UtxoEntry{
+			outpoint1200: entry1200.modifiedSpent,
+		},
+	}, {
+		name: "nil view entries are removed from view and do not affect cache",
 		viewEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint299:   nil,
+			outpoint299: nil,
+		},
+		cachedEntries: map[wire.OutPoint]*UtxoEntry{
+			outpoint299: entry299.unmodified,
+		},
+		wantViewEntries: map[wire.OutPoint]*UtxoEntry{},
+		wantCachedEntries: map[wire.OutPoint]*UtxoEntry{
+			outpoint299: entry299.unmodified,
+		},
+	}, {
+		// This covers the following scenarios:
+		//
+		// - unmodified spent view entries with no corresponding cache entry
+		//   are retained in the view and have no effect on the cache
+		// - unmodified spent view entries with a corresponding nil cache entry
+		//   are retained in the view and have no effect on the cache
+		// - unmodified spent view entries with a corresponding non-fresh
+		//   unmodified cache entry are removed from view and have no effect on
+		//   the cache
+		// - unmodified spent fresh view entries with a corresponding modified
+		//   unspent fresh cache entry are retained in the view and have no
+		//   effect on the cache
+		//
+		// NOTE: The case where there are unmodified spent view entries with
+		// corresponding unspent entries in the cache should really never happen
+		// in practice because any spending done in the view necessarily marks
+		// the entry spent, so the only realistic way for an unmodified spent
+		// view entry to exist would be for it to have come from the cache which
+		// means it would be spent in the cache too, but the cache will not have
+		// unmodified spent entries since those are pruned.
+		//
+		// Similarly, unmodified unspent fresh view entries never happen with
+		// the current implementation because the only way for a fresh view
+		// entry to exist is for it to have come from the cache, all fresh cache
+		// entries must necessarily also be marked modified, and cache entries
+		// are currently cloned directly into the view as is.
+		//
+		// However, they are tested here for completeness in order to ensure
+		// unmodified entries never affect the cache.
+		name: "several unmodified spent view entry combinations",
+		viewEntries: map[wire.OutPoint]*UtxoEntry{
+			outpoint299:   entry299.unmodifiedSpent,
+			outpoint1100:  entry1100.unmodifiedSpent,
+			outpoint1200:  entry1200.unmodifiedSpent,
+			outpoint85314: entry85314.unmodifiedSpentFresh,
+		},
+		cachedEntries: map[wire.OutPoint]*UtxoEntry{
+			outpoint1100:  nil,
+			outpoint1200:  entry1200.unmodified,
+			outpoint85314: entry85314.modifiedFresh,
+		},
+		wantViewEntries: map[wire.OutPoint]*UtxoEntry{
+			outpoint299:   entry299.unmodifiedSpent,
+			outpoint1100:  entry1100.unmodifiedSpent,
+			outpoint1200:  entry1200.unmodifiedSpent,
+			outpoint85314: entry85314.unmodifiedSpentFresh,
+		},
+		wantCachedEntries: map[wire.OutPoint]*UtxoEntry{
+			outpoint1100:  nil,
+			outpoint1200:  entry1200.unmodified,
+			outpoint85314: entry85314.modifiedFresh,
+		},
+	}, {
+		// This covers the following scenarios:
+		//
+		// - unmodified unspent view entries with no corresponding cache entry
+		//   are retained in the view and have no effect on the cache
+		// - unmodified unspent view entries with a corresponding nil cache
+		//   entry are retained in the view and have no effect on the cache
+		// - unmodified unspent view entries with a corresponding unmodified
+		//   cache entry are retained in the view and have no effect on the
+		//   cache
+		// - unmodified unspent fresh view entries with a corresponding modified
+		//   unspent fresh cache entry are retained in the view and have no
+		//   effect on the cache
+		//
+		// NOTE: Unmodified unspent fresh view entries never happen with the
+		// current implementation because the only way for a fresh view entry to
+		// exist is for it to have come from the cache, all fresh cache entries
+		// must necessarily also be marked modified, and cache entries are
+		// currently cloned directly into the view as is.  However, it is tested
+		// here for completeness.
+		name: "several unmodified unspent view entry combinations",
+		viewEntries: map[wire.OutPoint]*UtxoEntry{
+			outpoint299:   entry299.unmodified,
 			outpoint1100:  entry1100.unmodified,
-			outpoint1200:  entry1200.modifiedSpent,
-			outpoint85314: entry85314.modified,
+			outpoint1200:  entry1200.unmodified,
+			outpoint85314: entry85314.unmodifiedFresh,
 		},
 		cachedEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint1200: entry1200.unmodified,
+			outpoint1100:  nil,
+			outpoint1200:  entry1200.unmodified,
+			outpoint85314: entry85314.modifiedFresh,
 		},
-		// outpoint299 is removed from the view since the entry is nil.
-		// entry1100Unmodified remains in the view since it is unmodified.
-		// entry1200Spent is removed from the view since the entry is spent.
-		// entry85314Modified is removed from the view since it is modified and
-		// added to the cache.
 		wantViewEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint1100: entry1100.unmodified,
+			outpoint299:   entry299.unmodified,
+			outpoint1100:  entry1100.unmodified,
+			outpoint1200:  entry1200.unmodified,
+			outpoint85314: entry85314.unmodifiedFresh,
+		},
+		wantCachedEntries: map[wire.OutPoint]*UtxoEntry{
+			outpoint1100:  nil,
+			outpoint1200:  entry1200.unmodified,
+			outpoint85314: entry85314.modifiedFresh,
+		},
+	}, {
+		// This covers the following scenarios:
+		//
+		// - modified spent view entries with a corresponding nil cache entry
+		//   are removed from view and a noop to the cache
+		// - modified spent fresh view entries with a corresponding unspent
+		//   fresh cache entry makes the cache entry nil
+		// - modified spent view entries with a corresponding non-fresh
+		//   unmodified cache entry are removed from view and mark the cache
+		//   entry modified and spent
+		name: "several modified spent view entry combinations",
+		viewEntries: map[wire.OutPoint]*UtxoEntry{
+			outpoint299:  entry299.modifiedSpent,
+			outpoint1100: entry1100.modifiedSpentFresh,
+			outpoint1200: entry1200.modifiedSpent,
+		},
+		cachedEntries: map[wire.OutPoint]*UtxoEntry{
+			outpoint299:  nil,
+			outpoint1100: entry1100.modifiedFresh,
+			outpoint1200: entry1200.unmodified,
+		},
+		wantViewEntries: map[wire.OutPoint]*UtxoEntry{},
+		wantCachedEntries: map[wire.OutPoint]*UtxoEntry{
+			outpoint299:  nil,
+			outpoint1100: nil,
+			outpoint1200: entry1200.modifiedSpent,
 		},
-		// entry1200Spent remains in the cache but is now spent.
-		// entry85314Modified is added to the cache.
+	}, {
+		// This covers the following scenarios:
+		//
+		// - modified unspent view entries with no corresponding cache entry are
+		//   removed from view and added to the cache as fresh entries
+		// - modified unspent view entries with a corresponding unmodified cache
+		//   entry are removed from view and replace the cache entry making it
+		//   modified
+		// - modified unspent fresh view entries with a corresponding modified
+		//   fresh cache entry are removed from view and replace the cache entry
+		//   retaining its modified fresh status
+		// - modified unspent non-fresh view entries with a corresponding
+		//   modified non-fresh cache entry are removed from view and replace
+		//   the cache entry retaining its modified non-fresh status
+		name: "several modified unspent view entry combinations",
+		viewEntries: map[wire.OutPoint]*UtxoEntry{
+			outpoint299:   entry299.modified,
+			outpoint1100:  entry1100.modified,
+			outpoint1200:  entry1200.modifiedFresh,
+			outpoint85314: entry85314.modified,
+		},
+		cachedEntries: map[wire.OutPoint]*UtxoEntry{
+			outpoint1100:  entry1100.unmodified,
+			outpoint1200:  entry1200.modifiedFresh,
+			outpoint85314: entry85314.modified,
+		},
+		wantViewEntries: map[wire.OutPoint]*UtxoEntry{},
 		wantCachedEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint1200:  entry1200.modifiedSpent,
+			outpoint299:   entry299.modifiedFresh,
+			outpoint1100:  entry1100.modified,
+			outpoint1200:  entry1200.modifiedFresh,
 			outpoint85314: entry85314.modified,
 		},
 	}}
@@ -700,17 +857,26 @@ func TestCommit(t *testing.T) {
 		// Create a utxo cache with the cached entries specified by the test.
 		utxoCache := createTestUtxoCache(t, test.cachedEntries)
 
-		// Create a utxo cache with the view entries specified by the test.
+		// Create a utxo view with the entries specified by the test.  The view
+		// entries are cloned first in order to ensure the shared instances are
+		// not modified if the cache takes ownership of them.
+		viewEntries := make(map[wire.OutPoint]*UtxoEntry)
+		for k, v := range test.viewEntries {
+			viewEntries[k] = v.Clone()
+		}
 		view := &UtxoViewpoint{
 			cache:   utxoCache,
-			entries: test.viewEntries,
+			entries: viewEntries,
 		}
 
 		// Commit the view to the cache.
 		err := utxoCache.Commit(view)
-		if err != nil {
-			t.Fatalf("%q: unexpected error committing view to the cache: %v",
-				test.name, err)
+		if !errors.Is(err, test.err) {
+			t.Fatalf("%q: unexpected error committing view to the cache -- "+
+				"got %v (%[2]T), want %v (%[3]T)", test.name, err, test.err)
+		}
+		if test.err != nil {
+			continue
 		}
 
 		// Validate the cached entries after committing.

From 27051de37686e0d3bc54867dd596103fed13630d Mon Sep 17 00:00:00 2001
From: Dave Collins <davec@conformal.com>
Date: Tue, 13 Sep 2022 05:24:17 -0500
Subject: [PATCH 15/19] blockchain: Rework utxo cache add entry tests.

This reworks the utxo cache add entry tests to simplify them and make
them more flexible by moving the primary logic for all of the expected
results into the test definitions themselves as oppossed to special
casing depending on flags in the test body.
---
 internal/blockchain/utxocache_test.go | 85 +++++++++++----------------
 1 file changed, 33 insertions(+), 52 deletions(-)

diff --git a/internal/blockchain/utxocache_test.go b/internal/blockchain/utxocache_test.go
index f2eed94992..a1cdae858e 100644
--- a/internal/blockchain/utxocache_test.go
+++ b/internal/blockchain/utxocache_test.go
@@ -355,72 +355,53 @@ func TestAddEntry(t *testing.T) {
 	t.Parallel()
 
 	// Create test entries to be used throughout the tests.
-	outpoint, entry := outpoint299(), makeEntryStates(entry299())
-	entry.modified.amount++
+	outpoint, origEntry := outpoint299(), makeEntryStates(entry299())
+	mutatedEntry := makeEntryStates(entry299())
+	mutatedEntry.modified.amount++
+	mutatedEntry.modifiedFresh.amount++
 
+	type entriesMap map[wire.OutPoint]*UtxoEntry
 	tests := []struct {
-		name            string
-		existingEntries map[wire.OutPoint]*UtxoEntry
-		outpoint        wire.OutPoint
-		entry           *UtxoEntry
-		wantEntry       *UtxoEntry
+		name          string        // test description
+		cachedEntries entriesMap    // existing cache entires
+		outpoint      wire.OutPoint // outpoint for entry to add/update
+		entry         *UtxoEntry    // entry to add/update
+		wantEntry     *UtxoEntry    // expected entry in updated cache
+		wantCacheSize uint64        // expected size of updated cache
 	}{{
-		name:      "add an entry that does not already exist in the cache",
-		outpoint:  outpoint,
-		entry:     entry.unmodified,
-		wantEntry: entry.modifiedFresh,
+		name:          "entry not in cache adds modified fresh entry",
+		outpoint:      outpoint,
+		entry:         origEntry.unmodified,
+		wantEntry:     origEntry.modifiedFresh,
+		wantCacheSize: origEntry.modifiedFresh.size(),
 	}, {
-		name: "add an entry that overwrites an existing entry",
-		existingEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint: entry.unmodified,
-		},
-		outpoint:  outpoint,
-		entry:     entry.modified,
-		wantEntry: entry.modified,
+		name:          "existing cache entry is overwritten",
+		cachedEntries: entriesMap{outpoint: origEntry.unmodified},
+		outpoint:      outpoint,
+		entry:         mutatedEntry.modified,
+		wantEntry:     mutatedEntry.modified,
+		wantCacheSize: mutatedEntry.modified.size(),
 	}}
 
 	for _, test := range tests {
 		// Create a utxo cache with the existing entries specified by the test.
-		utxoCache := createTestUtxoCache(t, test.existingEntries)
-		wantTotalEntrySize := utxoCache.totalEntrySize
-
-		// Attempt to get an existing entry from the cache.  If it exists,
-		// subtract its size from the expected total entry size since it will be
-		// overwritten.
-		existingEntry := utxoCache.entries[test.outpoint]
-		if existingEntry != nil {
-			wantTotalEntrySize -= test.entry.size()
-		}
-
-		// Add the entry specified by the test.
-		utxoCache.addEntry(test.outpoint, test.entry)
-		wantTotalEntrySize += test.entry.size()
-
-		// Attempt to get the added entry from the cache.
-		cachedEntry := utxoCache.entries[test.outpoint]
-
-		// Validate that the added entry exists in the cache.
-		if cachedEntry == nil {
-			t.Fatalf("%q: expected entry for outpoint %v to exist in the cache",
-				test.name, test.outpoint)
-		}
+		utxoCache := createTestUtxoCache(t, test.cachedEntries)
 
-		// Validate that the entry is marked as modified.
-		if !cachedEntry.isModified() {
-			t.Fatalf("%q: unexpected modified flag -- got false, want true",
-				test.name)
-		}
+		// Add/update the entry specified by the test.
+		utxoCache.addEntry(test.outpoint, test.entry.Clone())
 
-		// Validate that the cached entry matches the expected entry.
-		if !reflect.DeepEqual(cachedEntry, test.wantEntry) {
-			t.Fatalf("%q: mismatched cached entry:\nwant: %+v\n got: %+v\n",
-				test.name, test.wantEntry, cachedEntry)
+		// Get the entry associated with the output from the cache and ensure it
+		// matches the expected one.
+		gotEntry := utxoCache.entries[test.outpoint]
+		if !reflect.DeepEqual(gotEntry, test.wantEntry) {
+			t.Fatalf("%q: unexpected entry after spend -- got: %+v, want %+v",
+				test.name, gotEntry, test.wantEntry)
 		}
 
 		// Validate that the total entry size was updated as expected.
-		if utxoCache.totalEntrySize != wantTotalEntrySize {
+		if utxoCache.totalEntrySize != test.wantCacheSize {
 			t.Fatalf("%q: unexpected total entry size -- got %v, want %v",
-				test.name, utxoCache.totalEntrySize, wantTotalEntrySize)
+				test.name, utxoCache.totalEntrySize, test.wantCacheSize)
 		}
 	}
 }

From 557452830e905597c2f4c49a3ad9db8097c87141 Mon Sep 17 00:00:00 2001
From: Dave Collins <davec@conformal.com>
Date: Tue, 13 Sep 2022 05:24:18 -0500
Subject: [PATCH 16/19] blockchain: Separate utxo cache vs view state.

The code prior to this change clones entries from the cache directly
into a view which means any cache entries that are marked modified
and/or fresh end up in the view with the flags set.  This leads to
slightly less efficient code and several non-obvious assumptions that
are easy to violate when making updates to views or the cache.

For example, modified cache entries end up in the view marked as
modified which means that even if they aren't actually modified, once
the view is committed, those entries end up needlessly updating the
cache with the new entry that is actually the exact same as the existing
one.  Another example is that cache entries that are marked fresh have
to retain that flag in the view in order to ensure they remain fresh in
the cache once that entry is replaced per the previous point.

It is perhaps worth noting that, in practice, there is not typically a
lot extra work due to the modified entries since almost all utxos loaded
into a view from the cache are ultimately modified in some way which
necessarily marks them modified anyway.  However, it is still
technically incorrect for them to be marked modified before they are
actually modified and does lead to some extra work that is not
necessary.

To address the aforementioned, this modifies the utxo cache to ensure
the cache state and view states are separate by updating the addEntry
and fetchEntry methods to ensure the modified and spent flags are
cleared on cloned entries loaded from the cache and and always set
correctly when adding entries to the cache.

The result is slightly more efficient code and removal of potential
footguns due to non-obvious assumptions about the internals of the view
and cache logic.
---
 internal/blockchain/utxocache.go      | 64 +++++++++++++++++++--------
 internal/blockchain/utxocache_test.go | 47 +++++++++++++++-----
 2 files changed, 83 insertions(+), 28 deletions(-)

diff --git a/internal/blockchain/utxocache.go b/internal/blockchain/utxocache.go
index e1195a55e6..37162b1ec5 100644
--- a/internal/blockchain/utxocache.go
+++ b/internal/blockchain/utxocache.go
@@ -274,26 +274,41 @@ func (c *UtxoCache) hitRatio() float64 {
 //
 // This function MUST be called with the cache lock held.
 func (c *UtxoCache) addEntry(outpoint wire.OutPoint, entry *UtxoEntry) {
+	// The entry will either be added to the cache when there is not already
+	// an existing entry for the specified outpoint or will overwrite the
+	// existing one when there is, but in either case the resulting cache
+	// entry is modified and thus needs to be marked as such.
+	entry.state |= utxoStateModified
+
 	// Attempt to get an existing entry from the cache.
 	cachedEntry := c.entries[outpoint]
 
-	// If an existing entry does not exist, the added entry should be marked as
-	// modified and fresh.
+	// Mark the entry as fresh, add it to the cache, and update the total
+	// cache size when there is not already an existing cache entry.
 	if cachedEntry == nil {
-		entry.state |= utxoStateModified | utxoStateFresh
+		entry.state |= utxoStateFresh
+		c.entries[outpoint] = entry
+		c.totalEntrySize += entry.size()
+		return
 	}
 
-	// Add the entry to the cache.  In the case that an entry already exists,
-	// the existing entry is overwritten.  All fields are overwritten because
-	// it's possible (although extremely unlikely) that the existing entry is
-	// being replaced by a different transaction with the same hash.  This is
-	// allowed so long as the previous transaction is fully spent.
-	c.entries[outpoint] = entry
-
-	// Update the total entry size of the cache.
-	if cachedEntry != nil {
-		c.totalEntrySize -= cachedEntry.size()
+	// There is an existing entry in the cache at this point.
+	//
+	// Ensure the state of whether or not the existing entry is fresh is
+	// maintained, overwrite the existing entry, and update the total cache size
+	// accordingly.
+	//
+	// All fields are overwritten because it's possible (although extremely
+	// unlikely) that the existing entry is being replaced by a different
+	// transaction with the same hash.  This is allowed so long as the previous
+	// transaction is fully spent.
+	if cachedEntry.isFresh() {
+		entry.state |= utxoStateFresh
+	} else {
+		entry.state &^= utxoStateFresh
 	}
+	c.entries[outpoint] = entry
+	c.totalEntrySize -= cachedEntry.size()
 	c.totalEntrySize += entry.size()
 }
 
@@ -329,20 +344,30 @@ func (c *UtxoCache) spendEntry(outpoint wire.OutPoint) {
 // the output exists in the cache, it is returned immediately.  Otherwise, it
 // fetches the output from the backend, caches it, and returns it to the
 // caller.  A cloned copy of the entry is returned so it can safely be mutated
-// by the caller without invalidating the cache.
+// by the caller without invalidating the cache.  The returned entry will not
+// have the modified or fresh flags set to ensure the cache state is separate
+// from the caller state.
 //
 // When there is no entry for the provided output, nil will be returned for both
 // the entry and the error.
 //
 // This function MUST be called with the cache lock held.
 func (c *UtxoCache) fetchEntry(outpoint wire.OutPoint) (*UtxoEntry, error) {
-	// If the cache already has the entry, return it immediately.  A cloned copy
-	// of the entry is returned so it can safely be mutated by the caller
-	// without invalidating the cache.
+	// Return a cloned copy of the cache entry when one exists for the provided
+	// output.  A cloned copy of the entry is returned so it can safely be
+	// mutated by the caller without invalidating the cache.
+	//
+	// Also clear the modified and fresh flags for the returned entry to ensure
+	// the cache state is separate from the view state.
 	var entry *UtxoEntry
 	if entry, found := c.entries[outpoint]; found {
 		c.hits++
-		return entry.Clone(), nil
+
+		clonedEntry := entry.Clone()
+		if clonedEntry != nil {
+			clonedEntry.state &^= utxoStateModified | utxoStateFresh
+		}
+		return clonedEntry, nil
 	}
 
 	// Increment cache misses.
@@ -367,6 +392,9 @@ func (c *UtxoCache) fetchEntry(outpoint wire.OutPoint) (*UtxoEntry, error) {
 	// Add the entry to the cache and return it.  A cloned copy of the entry is
 	// returned so it can safely be mutated by the caller without invalidating
 	// the cache.
+	//
+	// Also note that all entries loaded from the backend will not have any
+	// state flags set since they are memory only flags.
 	c.entries[outpoint] = entry
 	return entry.Clone(), nil
 }
diff --git a/internal/blockchain/utxocache_test.go b/internal/blockchain/utxocache_test.go
index a1cdae858e..b49ca29c0a 100644
--- a/internal/blockchain/utxocache_test.go
+++ b/internal/blockchain/utxocache_test.go
@@ -375,12 +375,33 @@ func TestAddEntry(t *testing.T) {
 		wantEntry:     origEntry.modifiedFresh,
 		wantCacheSize: origEntry.modifiedFresh.size(),
 	}, {
-		name:          "existing cache entry is overwritten",
+		name:          "existing !fresh cache entry w/ !fresh entry stays !fresh",
 		cachedEntries: entriesMap{outpoint: origEntry.unmodified},
 		outpoint:      outpoint,
 		entry:         mutatedEntry.modified,
 		wantEntry:     mutatedEntry.modified,
 		wantCacheSize: mutatedEntry.modified.size(),
+	}, {
+		name:          "existing !fresh cache entry with fresh entry stays !fresh",
+		cachedEntries: entriesMap{outpoint: origEntry.unmodified},
+		outpoint:      outpoint,
+		entry:         mutatedEntry.modifiedFresh,
+		wantEntry:     mutatedEntry.modified,
+		wantCacheSize: mutatedEntry.modified.size(),
+	}, {
+		name:          "existing fresh cache entry with !fresh entry stays fresh",
+		cachedEntries: entriesMap{outpoint: origEntry.modifiedFresh},
+		outpoint:      outpoint,
+		entry:         mutatedEntry.modified,
+		wantEntry:     mutatedEntry.modifiedFresh,
+		wantCacheSize: mutatedEntry.modifiedFresh.size(),
+	}, {
+		name:          "existing fresh cache entry with fresh entry stays fresh",
+		cachedEntries: entriesMap{outpoint: origEntry.modifiedFresh},
+		outpoint:      outpoint,
+		entry:         mutatedEntry.modifiedFresh,
+		wantEntry:     mutatedEntry.modifiedFresh,
+		wantCacheSize: mutatedEntry.modifiedFresh.size(),
 	}}
 
 	for _, test := range tests {
@@ -699,10 +720,8 @@ func TestCommit(t *testing.T) {
 		// unmodified spent entries since those are pruned.
 		//
 		// Similarly, unmodified unspent fresh view entries never happen with
-		// the current implementation because the only way for a fresh view
-		// entry to exist is for it to have come from the cache, all fresh cache
-		// entries must necessarily also be marked modified, and cache entries
-		// are currently cloned directly into the view as is.
+		// the current implementation because views never set the fresh flag and
+		// the flag is cleared on cloned entries that come from the cache.
 		//
 		// However, they are tested here for completeness in order to ensure
 		// unmodified entries never affect the cache.
@@ -744,11 +763,9 @@ func TestCommit(t *testing.T) {
 		//   effect on the cache
 		//
 		// NOTE: Unmodified unspent fresh view entries never happen with the
-		// current implementation because the only way for a fresh view entry to
-		// exist is for it to have come from the cache, all fresh cache entries
-		// must necessarily also be marked modified, and cache entries are
-		// currently cloned directly into the view as is.  However, it is tested
-		// here for completeness.
+		// current implementation because views never set the fresh flag and the
+		// flag is cleared on cloned entries that come from the cache.  However,
+		// it is tested here for completeness.
 		name: "several unmodified unspent view entry combinations",
 		viewEntries: map[wire.OutPoint]*UtxoEntry{
 			outpoint299:   entry299.unmodified,
@@ -782,6 +799,11 @@ func TestCommit(t *testing.T) {
 		// - modified spent view entries with a corresponding non-fresh
 		//   unmodified cache entry are removed from view and mark the cache
 		//   entry modified and spent
+		//
+		// NOTE: Modified spent fresh view entries never happen with the current
+		// implementation because views never set the fresh flag and the flag is
+		// cleared on cloned entries that come from the cache.  However, it is
+		// tested here for completeness.
 		name: "several modified spent view entry combinations",
 		viewEntries: map[wire.OutPoint]*UtxoEntry{
 			outpoint299:  entry299.modifiedSpent,
@@ -813,6 +835,11 @@ func TestCommit(t *testing.T) {
 		// - modified unspent non-fresh view entries with a corresponding
 		//   modified non-fresh cache entry are removed from view and replace
 		//   the cache entry retaining its modified non-fresh status
+		//
+		// NOTE: Modified unspent fresh view entries never happen with the
+		// current implementation because views never set the fresh flag and the
+		// flag is cleared on cloned entries that come from the cache.  However,
+		// it is tested here for completeness.
 		name: "several modified unspent view entry combinations",
 		viewEntries: map[wire.OutPoint]*UtxoEntry{
 			outpoint299:   entry299.modified,

From 561c679fef5ff4fa70f56db1e43f8429ae74ed8f Mon Sep 17 00:00:00 2001
From: Dave Collins <davec@conformal.com>
Date: Tue, 13 Sep 2022 05:24:19 -0500
Subject: [PATCH 17/19] blockchain: Improve utxo cache spend robustness.

This modifies the utxo cache commit logic to ensure more robust handling
of spent outputs including the addition of a double spend assertion and
adds tests to ensure the new behavior works as intended.

In particular, it updates the spendEntry method to attempt to load any
entries that are not already in the cache from backend which is
necessary since lack of an entry does not always guarantee the entry is
not in the backend in the case of a hard to hit corner case involving a
mix of reorganizations and unclean shutdowns.  It also adds a new double
spend assertion while here since it's good practice to be paranoid and
double check assumptions when it comes to potential double spends.

While it is certainly possible to tackle the specific aforementioned
case in other ways, this approach was chosen because it is an overall
robust solution that ensures proper behavior regardless of other
independent assumptions that might be introduced in future code updates.
---
 internal/blockchain/utxocache.go      | 142 +++++++++++++++++++++-----
 internal/blockchain/utxocache_test.go |  84 ++++++++++++---
 2 files changed, 184 insertions(+), 42 deletions(-)

diff --git a/internal/blockchain/utxocache.go b/internal/blockchain/utxocache.go
index 37162b1ec5..b0089c04cf 100644
--- a/internal/blockchain/utxocache.go
+++ b/internal/blockchain/utxocache.go
@@ -314,17 +314,91 @@ func (c *UtxoCache) addEntry(outpoint wire.OutPoint, entry *UtxoEntry) {
 
 // spendEntry marks the specified output as spent.
 //
+// When the provided output does not already have an associated entry in the
+// cache, the cache will be populated with the entry from the backend (if one
+// exists) and marked spent to ensure the utxo is later removed from the backend
+// on the next cache flush.
+//
+// An error will be returned in the case the output exists in the cache and is
+// already spent.
+//
 // This function MUST be called with the cache lock held.
-func (c *UtxoCache) spendEntry(outpoint wire.OutPoint) {
-	// Attempt to get an existing entry from the cache.
-	cachedEntry := c.entries[outpoint]
+func (c *UtxoCache) spendEntry(outpoint wire.OutPoint) error {
+	// Attempt to get the entry associated with the output from the cache.
+	//
+	// In the case there is a nil entry in the cache for the output, it does not
+	// exist in the backend.  This is because either an attempt to load the utxo
+	// from the backend was made and the nil entry was added to the cache to
+	// avoid further lookups against the backend when the output is known to not
+	// exist or the output was added to the cache and spent in between flushes
+	// to the backend and thus has been pruned.
+	//
+	// In either case, the cache does not need to be updated, so there is
+	// nothing more to do.
+	cachedEntry, found := c.entries[outpoint]
+	if found && cachedEntry == nil {
+		return nil
+	}
 
-	// If the entry is nil or already spent, return immediately.
-	if cachedEntry == nil || cachedEntry.IsSpent() {
-		return
+	// Sanity check double spends of existing entries.  This should be
+	// impossible to hit since a view should never be committed until after
+	// double spend checks have already been done.  However, it is good practice
+	// to be paranoid and double check assumptions when it comes to potential
+	// double spends.
+	if cachedEntry != nil && cachedEntry.IsSpent() {
+		return AssertError(fmt.Sprintf("attempt to double spend output %v in "+
+			"view commit", outpoint))
+	}
+
+	// Attempt to repopulate the cache from the backend when there is no entry
+	// for the output in the cache.  This is necessary because the associated
+	// utxo might exist in the backend but not have been loaded into the cache
+	// yet or have otherwise been evicted.
+	//
+	// Thus, in order to ensure the utxo is removed from the backend on the next
+	// cache flush in the case it exists, the associated cache entry from the
+	// backend is loaded and marked spent.
+	if !found {
+		// Account for the cache miss and attempt to load the entry for the utxo
+		// from the backend.
+		//
+		// Notice that this intentionally attempts to load the entry from the
+		// backend directly as opposed to using the normal cache entry fetching
+		// method since that involves querying the cache again which would be
+		// wasteful since it is already known to not exist and also employs
+		// slightly different logic regarding populating the cache.
+		//
+		// NOTE: Missing backend entries are not an error.
+		c.misses++
+		backendEntry, err := c.backend.FetchEntry(outpoint)
+		if err != nil {
+			return err
+		}
+
+		// The cache does not need to be updated when the associated utxo does
+		// not exist in the backend, so there is nothing more to do in that
+		// case.
+		//
+		// Note that a nil entry is not added to cache for the output here since
+		// there should realistically never be any future lookups for the spent
+		// output given that would be a double spend.
+		if backendEntry == nil {
+			return nil
+		}
+
+		// Add the entry to the cache and update its total entry size.
+		c.entries[outpoint] = backendEntry
+		c.totalEntrySize += backendEntry.size()
+
+		// Mark the output as spent and modified.
+		backendEntry.Spend()
+		return nil
 	}
 
-	// If the entry is fresh, and is now being spent, it can safely be removed.
+	// Remove the entry associated with the output from the cache when the
+	// backend does not need to be updated because the backend does not have an
+	// associated utxo (aka the cache entry is fresh).
+	//
 	// This is an optimization to skip writing to the backend for outputs that
 	// are added and spent in between flushes to the backend.
 	if cachedEntry.isFresh() {
@@ -333,11 +407,12 @@ func (c *UtxoCache) spendEntry(outpoint wire.OutPoint) {
 		// and avoid querying the backend.
 		c.entries[outpoint] = nil
 		c.totalEntrySize -= cachedEntry.size()
-		return
+		return nil
 	}
 
 	// Mark the output as spent and modified.
 	cachedEntry.Spend()
+	return nil
 }
 
 // fetchEntry returns the specified transaction output from the utxo set.  If
@@ -471,9 +546,12 @@ func (c *UtxoCache) FetchStats(bestHash *chainhash.Hash, bestHeight uint32) (*Ut
 //
 // This function is safe for concurrent access.
 func (c *UtxoCache) Commit(view *UtxoViewpoint) error {
+	defer c.cacheLock.Unlock()
 	c.cacheLock.Lock()
+
 	for outpoint, entry := range view.entries {
-		// If the entry is nil, delete it from the view and continue.
+		// There is nothing to update in the cache when the view entry is nil,
+		// so remove it from the view and continue to the next entry.
 		if entry == nil {
 			delete(view.entries, outpoint)
 			continue
@@ -485,34 +563,42 @@ func (c *UtxoCache) Commit(view *UtxoViewpoint) error {
 			continue
 		}
 
-		// If the entry is modified and spent, mark it as spent in the cache and
-		// then delete it from the view.
+		// ---------------------------------------------------------------------
+		// NOTE: As an optimization in order to avoid an additional allocation,
+		// the cache takes ownership of all modified entries from the view.
+		//
+		// Thus, unless there is an error preventing the cache from taking
+		// ownership and ultimately causing the commit to fail, the entry will
+		// be removed from the view in all remaining paths to ensure that it is
+		// not mutated by the caller after being added to the cache.
+		//
+		// This does cause the view to refetch the entry if it is requested
+		// again after being removed.  However, this only really occurs during
+		// reorgs, whereas committing the view to the cache happens with every
+		// connected block, so this optimizes for the much more common case.
+		// ---------------------------------------------------------------------
+
+		// Mark the spent view entry as modified and spent in the cache and
+		// remove it from the view.
+		//
+		// This might entail populating the cache with the unspent output from
+		// the backend when there is not already a corresponding entry in the
+		// cache to ensure the utxo is later removed from the backend on the
+		// next cache flush.
 		if entry.IsSpent() {
-			// Mark the entry as spent in the cache.
-			c.spendEntry(outpoint)
+			if err := c.spendEntry(outpoint); err != nil {
+				return err
+			}
 
-			// Delete the entry from the view.
 			delete(view.entries, outpoint)
 			continue
 		}
 
-		// If we passed all of the conditions above, the entry is modified or
-		// fresh, but not spent, and should be added to the cache.
+		// Update the existing entry in the cache or add a new one whenever the
+		// view entry is both modified and unspent and remove it from the view.
 		c.addEntry(outpoint, entry)
-
-		// All entries that are added to the cache should be removed from the
-		// provided view.  This is an optimization to allow the cache to take
-		// ownership of the entry from the view and avoid an additional
-		// allocation.  It is removed from the view to ensure that it is not
-		// mutated by the caller after being added to the cache.
-		//
-		// This does cause the view to refetch the entry if it is requested
-		// again after being removed.  However, this only really occurs during
-		// reorgs, whereas committing the view to the cache happens with every
-		// connected block, so this optimizes for the much more common case.
 		delete(view.entries, outpoint)
 	}
-	c.cacheLock.Unlock()
 
 	return nil
 }
diff --git a/internal/blockchain/utxocache_test.go b/internal/blockchain/utxocache_test.go
index b49ca29c0a..95813a8d6e 100644
--- a/internal/blockchain/utxocache_test.go
+++ b/internal/blockchain/utxocache_test.go
@@ -432,6 +432,9 @@ func TestAddEntry(t *testing.T) {
 func TestSpendEntry(t *testing.T) {
 	t.Parallel()
 
+	// Create a test backend.
+	backend := createTestUtxoBackend(t)
+
 	// Create test entries to be used throughout the tests.
 	outpoint, entry := outpoint299(), makeEntryStates(entry299())
 
@@ -443,23 +446,29 @@ func TestSpendEntry(t *testing.T) {
 		outpoint       wire.OutPoint // outpoint for the entry to spend
 		wantEntry      *UtxoEntry    // expected entry in cache after spend
 		wantCacheSize  uint64        // expected size of the cache after spend
+		err            error         // expected error
 	}{{
+		name:          "spending cache entry already marked spent asserts",
+		cachedEntries: entriesMap{outpoint: entry.modifiedSpent},
+		outpoint:      outpoint,
+		err:           AssertError(""),
+	}, {
 		name:          "spending nil (aka pruned) cache entry is a noop",
 		cachedEntries: entriesMap{outpoint: nil},
 		outpoint:      outpoint,
 		wantEntry:     nil,
 		wantCacheSize: 0,
 	}, {
-		name:          "spending entry not in cache is a noop",
+		name:          "spending entry not in cache queries backend (no utxo)",
 		outpoint:      outpoint,
 		wantEntry:     nil,
 		wantCacheSize: 0,
 	}, {
-		name:          "spending cache entry already marked spent is a noop",
-		cachedEntries: entriesMap{outpoint: entry.modifiedSpent},
-		outpoint:      outpoint,
-		wantEntry:     entry.modifiedSpent,
-		wantCacheSize: entry.modifiedSpent.size(),
+		name:           "spending entry not in cache queries backend (with utxo)",
+		outpoint:       outpoint,
+		backendEntries: entriesMap{outpoint: entry.modified},
+		wantEntry:      entry.modifiedSpent,
+		wantCacheSize:  entry.modifiedSpent.size(),
 	}, {
 		name:          "spending cache entry marked fresh makes it nil",
 		cachedEntries: entriesMap{outpoint: entry.modifiedFresh},
@@ -477,9 +486,25 @@ func TestSpendEntry(t *testing.T) {
 	for _, test := range tests {
 		// Create a utxo cache with the existing entries specified by the test.
 		utxoCache := createTestUtxoCache(t, test.cachedEntries)
+		utxoCache.backend = backend
 
-		// Spend the entry specified by the test.
-		utxoCache.spendEntry(test.outpoint)
+		// Add entries specified by the test to the test backend.
+		err := backend.PutUtxos(test.backendEntries, &UtxoSetState{})
+		if err != nil {
+			t.Fatalf("%q: unexpected error adding entries to test backend: %v",
+				test.name, err)
+		}
+
+		// Spend the entry specified by the test and ensure the error matches
+		// the expected one.
+		err = utxoCache.spendEntry(test.outpoint)
+		if !errors.Is(err, test.err) {
+			t.Fatalf("%q: unexpected error spending outpoint %v -- got %v, "+
+				"want %v (%[4]T)", test.name, test.outpoint, err, test.err)
+		}
+		if test.err != nil {
+			continue
+		}
 
 		// Get the entry associated with the output from the cache and ensure it
 		// matches the expected one.
@@ -660,6 +685,9 @@ func TestFetchEntries(t *testing.T) {
 func TestCommit(t *testing.T) {
 	t.Parallel()
 
+	// Create a test backend.
+	backend := createTestUtxoBackend(t)
+
 	// Create test entries to be used throughout the tests.
 	outpoint299, entry299 := outpoint299(), makeEntryStates(entry299())
 	outpoint1100, entry1100 := outpoint1100(), makeEntryStates(entry1100())
@@ -670,21 +698,19 @@ func TestCommit(t *testing.T) {
 		name              string                       // test description
 		viewEntries       map[wire.OutPoint]*UtxoEntry // view to commit
 		cachedEntries     map[wire.OutPoint]*UtxoEntry // existing cache entries
+		backendEntries    map[wire.OutPoint]*UtxoEntry // existing backend entries
 		wantViewEntries   map[wire.OutPoint]*UtxoEntry // expected committed view
 		wantCachedEntries map[wire.OutPoint]*UtxoEntry // expected committed cache
 		err               error                        // expected error
 	}{{
-		name: "modified spent view entry w/ existing spent cache entry is noop",
+		name: "modified spent view entry w/ existing spent cache entry asserts",
 		viewEntries: map[wire.OutPoint]*UtxoEntry{
 			outpoint1200: entry1200.modifiedSpent,
 		},
 		cachedEntries: map[wire.OutPoint]*UtxoEntry{
 			outpoint1200: entry1200.modifiedSpent,
 		},
-		wantViewEntries: map[wire.OutPoint]*UtxoEntry{},
-		wantCachedEntries: map[wire.OutPoint]*UtxoEntry{
-			outpoint1200: entry1200.modifiedSpent,
-		},
+		err: AssertError(""),
 	}, {
 		name: "nil view entries are removed from view and do not affect cache",
 		viewEntries: map[wire.OutPoint]*UtxoEntry{
@@ -821,6 +847,28 @@ func TestCommit(t *testing.T) {
 			outpoint1100: nil,
 			outpoint1200: entry1200.modifiedSpent,
 		},
+	}, {
+		// This covers the following scenarios:
+		//
+		// - modified spent view entries without a corresponding cache entry nor
+		//   corresponding backend entry are removed from the view and a noop to
+		//   the cache
+		// - modified spent view entries without a corresponding cache entry
+		//   but with a corresponding backend entry loads the entry from the
+		//   backend, spends it, and adds the spent entry to the cache
+		name: "modified spent view entries not in backend",
+		viewEntries: map[wire.OutPoint]*UtxoEntry{
+			outpoint299:  entry299.modifiedSpent,
+			outpoint1100: entry1100.modifiedSpent,
+		},
+		cachedEntries: map[wire.OutPoint]*UtxoEntry{},
+		backendEntries: map[wire.OutPoint]*UtxoEntry{
+			outpoint1100: entry1100.modified,
+		},
+		wantViewEntries: map[wire.OutPoint]*UtxoEntry{},
+		wantCachedEntries: map[wire.OutPoint]*UtxoEntry{
+			outpoint1100: entry1100.modifiedSpent,
+		},
 	}, {
 		// This covers the following scenarios:
 		//
@@ -864,6 +912,14 @@ func TestCommit(t *testing.T) {
 	for _, test := range tests {
 		// Create a utxo cache with the cached entries specified by the test.
 		utxoCache := createTestUtxoCache(t, test.cachedEntries)
+		utxoCache.backend = backend
+
+		// Add entries specified by the test to the test backend.
+		err := backend.PutUtxos(test.backendEntries, &UtxoSetState{})
+		if err != nil {
+			t.Fatalf("%q: unexpected error adding entries to test backend: %v",
+				test.name, err)
+		}
 
 		// Create a utxo view with the entries specified by the test.  The view
 		// entries are cloned first in order to ensure the shared instances are
@@ -878,7 +934,7 @@ func TestCommit(t *testing.T) {
 		}
 
 		// Commit the view to the cache.
-		err := utxoCache.Commit(view)
+		err = utxoCache.Commit(view)
 		if !errors.Is(err, test.err) {
 			t.Fatalf("%q: unexpected error committing view to the cache -- "+
 				"got %v (%[2]T), want %v (%[3]T)", test.name, err, test.err)

From 06ab70edf8a3ed86d06f3ae2b2dd7796c300f03b Mon Sep 17 00:00:00 2001
From: Dave Collins <davec@conformal.com>
Date: Tue, 13 Sep 2022 05:24:20 -0500
Subject: [PATCH 18/19] blockchain: Split regular/stake view tx connect.

This splits the logic for handling the connection of transactions from
the regular tree and stake tree to a utxo view by introducing individual
methods for them and updating the callers accordingly.

The primary motivation for this change is that the regular and stake
transaction trees do not have identical logic in terms of the ability to
spend outputs earlier in the block and it is more efficient to avoid
continuously checking for special conditions across all transactions
when only certain types can possibly be in each respective tree.

Splitting this logic also paves the way for future commits to take
advantage of the aforementioned spending semantics to optimize the utxo
cache.
---
 internal/blockchain/utxoviewpoint.go | 188 +++++++++++++++++++--------
 internal/blockchain/validate.go      |   6 +-
 2 files changed, 140 insertions(+), 54 deletions(-)

diff --git a/internal/blockchain/utxoviewpoint.go b/internal/blockchain/utxoviewpoint.go
index 6dee5770f0..7f4cb54821 100644
--- a/internal/blockchain/utxoviewpoint.go
+++ b/internal/blockchain/utxoviewpoint.go
@@ -216,26 +216,45 @@ func (view *UtxoViewpoint) PriorityInput(prevOut *wire.OutPoint) (int64, int64,
 	return 0, 0, false
 }
 
-// connectTransaction updates the view by adding all new utxos created by the
-// passed transaction and marking all utxos that the transaction spends as
-// spent.  In addition, when the 'stxos' argument is not nil, it will be updated
-// to append an entry for each spent txout.  An error will be returned if the
-// view does not contain the required utxos.
-func (view *UtxoViewpoint) connectTransaction(tx *dcrutil.Tx, blockHeight int64,
-	blockIndex uint32, stxos *[]spentTxOut, isTreasuryEnabled bool) error {
-
-	// Treasurybase transactions don't have any inputs to spend.
+// utxoEntryToSpentTxOut converts the provided utxo entry to a spent txout for
+// use in tracking all spent txouts for the spend journal.
+func utxoEntryToSpentTxOut(entry *UtxoEntry) spentTxOut {
+	// Populate the stxo details using the utxo entry.
+	return spentTxOut{
+		amount:        entry.Amount(),
+		pkScript:      entry.PkScript(),
+		ticketMinOuts: entry.ticketMinOuts,
+		blockHeight:   uint32(entry.BlockHeight()),
+		blockIndex:    entry.BlockIndex(),
+		scriptVersion: entry.ScriptVersion(),
+		packedFlags: encodeFlags(entry.IsCoinBase(), entry.HasExpiry(),
+			entry.TransactionType()),
+	}
+}
+
+// connectStakeTransaction updates the view by adding all new utxos created by
+// the passed transaction from the stake transaction tree and marking all utxos
+// that the transaction spends as spent.  In addition, when the 'stxos' argument
+// is not nil, it will be updated to append an entry for each spent txout.  An
+// error will be returned if the view does not contain the required utxos.
+func (view *UtxoViewpoint) connectStakeTransaction(tx *dcrutil.Tx,
+	blockHeight int64, blockIndex uint32, stxos *[]spentTxOut,
+	isTreasuryEnabled bool) error {
+
+	// Treasurybase transactions don't have any inputs to spend or outputs to
+	// add.
 	//
-	// NOTE: This check MUST come before the coinbase check because a
-	// treasurybase is identified as a coinbase as of the time this comment is
-	// being written.
+	// NOTE: This check relies on the code already having validated that the
+	// first transaction, and only the first transaction, in the stake tree is
+	// the required treasurybase when the treasury agenda is active.
 	msgTx := tx.MsgTx()
-	if isTreasuryEnabled && standalone.IsTreasuryBase(msgTx) {
+	if isTreasuryEnabled && blockIndex == 0 {
 		return nil
 	}
 
-	// Coinbase transactions don't have any inputs to spend.
-	if standalone.IsCoinBaseTx(msgTx, isTreasuryEnabled) {
+	// Treasury spends don't have any inputs to spend.
+	isTreasurySpend := isTreasuryEnabled && stake.IsTSpend(msgTx)
+	if isTreasurySpend {
 		// Add the transaction's outputs as available utxos.
 		view.AddTxOuts(tx, blockHeight, blockIndex, isTreasuryEnabled)
 		return nil
@@ -244,41 +263,92 @@ func (view *UtxoViewpoint) connectTransaction(tx *dcrutil.Tx, blockHeight int64,
 	// Spend the referenced utxos by marking them spent in the view and, if a
 	// slice was provided for the spent txout details, append an entry to it.
 	isVote := stake.IsSSGen(msgTx)
-	isTreasurySpend := isTreasuryEnabled && stake.IsTSpend(msgTx)
 	for txInIdx, txIn := range msgTx.TxIn {
 		// Ignore stakebase since it has no input.
 		if isVote && txInIdx == 0 {
 			continue
 		}
 
-		// Ignore treasury spends since they have no inputs.
-		if isTreasurySpend {
-			continue
+		// Ensure the referenced utxo exists in the view.  This should never
+		// happen unless a bug is introduced in the code.
+		prevOut := &txIn.PreviousOutPoint
+		entry := view.entries[*prevOut]
+		if entry == nil {
+			return AssertError(fmt.Sprintf("view missing input %v", *prevOut))
+		}
+
+		// Only create the stxo details if requested.
+		if stxos != nil {
+			// Populate the stxo details using the utxo entry.
+			*stxos = append(*stxos, utxoEntryToSpentTxOut(entry))
+		}
+
+		// Mark the entry as spent.  This is not done until after the relevant
+		// details have been accessed since spending it might clear the fields
+		// from memory in the future.
+		entry.Spend()
+	}
+
+	// Add the transaction's outputs as available utxos.
+	view.AddTxOuts(tx, blockHeight, blockIndex, isTreasuryEnabled)
+
+	return nil
+}
+
+// connectStakeTransactions updates the view by adding all new utxos created by
+// the transactions in the stake transaction tree of the block and marking all
+// utxos those same transactions spend as spent.  In addition, when the 'stxos'
+// argument is not nil, it will be updated to append an entry for each spent
+// txout.  An error will be returned if the view does not contain the required
+// utxos.
+func (view *UtxoViewpoint) connectStakeTransactions(block *dcrutil.Block,
+	stxos *[]spentTxOut, isTreasuryEnabled bool) error {
+
+	// Connect all of the transactions in the stake transaction tree.
+	for i, tx := range block.STransactions() {
+		err := view.connectStakeTransaction(tx, block.Height(), uint32(i),
+			stxos, isTreasuryEnabled)
+		if err != nil {
+			return err
 		}
+	}
+
+	return nil
+}
+
+// connectRegularTransaction updates the view by adding all new utxos created by
+// the passed transaction from the regular transaction tree and marking all
+// utxos that the transaction spends as spent.  In addition, when the 'stxos'
+// argument is not nil, it will be updated to append an entry for each spent
+// txout.  An error will be returned if the view does not contain the required
+// utxos.
+func (view *UtxoViewpoint) connectRegularTransaction(tx *dcrutil.Tx,
+	blockHeight int64, blockIndex uint32, stxos *[]spentTxOut,
+	isTreasuryEnabled bool) error {
 
+	// Coinbase transactions don't have any inputs to spend.
+	msgTx := tx.MsgTx()
+	if standalone.IsCoinBaseTx(msgTx, isTreasuryEnabled) {
+		// Add the transaction's outputs as available utxos.
+		view.AddTxOuts(tx, blockHeight, blockIndex, isTreasuryEnabled)
+		return nil
+	}
+
+	// Spend the referenced utxos by marking them spent in the view and, if a
+	// slice was provided for the spent txout details, append an entry to it.
+	for _, txIn := range msgTx.TxIn {
 		// Ensure the referenced utxo exists in the view.  This should never
-		// happen unless there is a bug is introduced in the code.
-		entry := view.entries[txIn.PreviousOutPoint]
+		// happen unless a bug is introduced in the code.
+		prevOut := &txIn.PreviousOutPoint
+		entry := view.entries[*prevOut]
 		if entry == nil {
-			return AssertError(fmt.Sprintf("view missing input %v",
-				txIn.PreviousOutPoint))
+			return AssertError(fmt.Sprintf("view missing input %v", *prevOut))
 		}
 
 		// Only create the stxo details if requested.
 		if stxos != nil {
 			// Populate the stxo details using the utxo entry.
-			var stxo = spentTxOut{
-				amount:        entry.Amount(),
-				pkScript:      entry.PkScript(),
-				ticketMinOuts: entry.ticketMinOuts,
-				blockHeight:   uint32(entry.BlockHeight()),
-				blockIndex:    entry.BlockIndex(),
-				scriptVersion: entry.ScriptVersion(),
-				packedFlags: encodeFlags(entry.IsCoinBase(), entry.HasExpiry(),
-					entry.TransactionType()),
-			}
-
-			*stxos = append(*stxos, stxo)
+			*stxos = append(*stxos, utxoEntryToSpentTxOut(entry))
 		}
 
 		// Mark the entry as spent.  This is not done until after the relevant
@@ -293,6 +363,27 @@ func (view *UtxoViewpoint) connectTransaction(tx *dcrutil.Tx, blockHeight int64,
 	return nil
 }
 
+// connectRegularTransactions updates the view by adding all new utxos created
+// by the transactions in the regular transaction tree of the block and marking
+// all utxos those same transactions spend as spent.  In addition, when the
+// 'stxos' argument is not nil, it will be updated to append an entry for each
+// spent txout.  An error will be returned if the view does not contain the
+// required utxos.
+func (view *UtxoViewpoint) connectRegularTransactions(block *dcrutil.Block,
+	stxos *[]spentTxOut, isTreasuryEnabled bool) error {
+
+	// Connect all of the transactions in the regular transaction tree.
+	for i, tx := range block.Transactions() {
+		err := view.connectRegularTransaction(tx, block.Height(), uint32(i),
+			stxos, isTreasuryEnabled)
+		if err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
 // disconnectTransactions updates the view by removing all utxos created by the
 // transactions in either the regular or stake tree of the block, depending on
 // the flag, and unspending all of the txos spent by those same transactions by
@@ -538,19 +629,13 @@ func (view *UtxoViewpoint) connectBlock(db database.DB, block, parent *dcrutil.B
 	// of transactions created in the regular tree of the same block, which is
 	// important since the regular tree may be disapproved by the subsequent
 	// block while the stake tree must remain valid.
-	for i, stx := range block.STransactions() {
-		err := view.connectTransaction(stx, block.Height(), uint32(i),
-			stxos, isTreasuryEnabled)
-		if err != nil {
-			return err
-		}
+	err = view.connectStakeTransactions(block, stxos, isTreasuryEnabled)
+	if err != nil {
+		return err
 	}
-	for i, tx := range block.Transactions() {
-		err := view.connectTransaction(tx, block.Height(), uint32(i),
-			stxos, isTreasuryEnabled)
-		if err != nil {
-			return err
-		}
+	err = view.connectRegularTransactions(block, stxos, isTreasuryEnabled)
+	if err != nil {
+		return err
 	}
 
 	// Update the best hash for view to include this block since all of its
@@ -615,12 +700,9 @@ func (view *UtxoViewpoint) disconnectBlock(block, parent *dcrutil.Block,
 			return err
 		}
 
-		for i, tx := range parent.Transactions() {
-			err := view.connectTransaction(tx, parent.Height(),
-				uint32(i), nil, isTreasuryEnabled)
-			if err != nil {
-				return err
-			}
+		err = view.connectRegularTransactions(parent, nil, isTreasuryEnabled)
+		if err != nil {
+			return err
 		}
 	}
 
diff --git a/internal/blockchain/validate.go b/internal/blockchain/validate.go
index 19dbf498f6..d4faf9ae7c 100644
--- a/internal/blockchain/validate.go
+++ b/internal/blockchain/validate.go
@@ -3517,7 +3517,11 @@ func (b *BlockChain) checkTransactionsAndConnect(inputFees dcrutil.Amount, node
 		//
 		// Also, update the passed spent txos slice to contain an entry for each
 		// output the transaction spends.
-		err = view.connectTransaction(tx, node.height, uint32(idx), stxos,
+		connectTransaction := view.connectRegularTransaction
+		if stakeTree {
+			connectTransaction = view.connectStakeTransaction
+		}
+		err = connectTransaction(tx, node.height, uint32(idx), stxos,
 			isTreasuryEnabled)
 		if err != nil {
 			return err

From 8cfdcad399aafb188afc26c954b67d70dd4970bb Mon Sep 17 00:00:00 2001
From: Dave Collins <davec@conformal.com>
Date: Tue, 13 Sep 2022 05:24:20 -0500
Subject: [PATCH 19/19] blockchain: Bypass utxo cache for zero conf spends.

This updates the utxo view and cache handling to detect zero
confirmation spends in the regular transaction tree (outputs that are
spent by the inputs of transactions later in the same tree) and bypass
the cache when committing the view.

This is desirable because such outputs never exist beyond the scope of
the view and therefore are guaranteed to never have any cache entries.
In other words, any attempts to spend them in cache will necessarily
result in a cache miss.

In order to implement the logic, a new state bit flag to track zero
confirmation spends is introduced along with code to appropriately set
and clear the flag when connecting and disconnection transactions.  The
bit flag is then used to bypass the cache spend when set.

Finally, cache commit tests are included to ensure proper functionality.
---
 internal/blockchain/utxocache.go      | 22 ++++++++
 internal/blockchain/utxocache_test.go | 58 ++++++++++++++------
 internal/blockchain/utxoentry.go      | 28 +++++++---
 internal/blockchain/utxoviewpoint.go  | 76 +++++++++++++++++++++------
 internal/blockchain/validate.go       | 28 +++++++---
 5 files changed, 165 insertions(+), 47 deletions(-)

diff --git a/internal/blockchain/utxocache.go b/internal/blockchain/utxocache.go
index b0089c04cf..1a0889a510 100644
--- a/internal/blockchain/utxocache.go
+++ b/internal/blockchain/utxocache.go
@@ -578,6 +578,28 @@ func (c *UtxoCache) Commit(view *UtxoViewpoint) error {
 		// connected block, so this optimizes for the much more common case.
 		// ---------------------------------------------------------------------
 
+		// There is nothing to update in the cache when the output is spent by a
+		// transaction later in the regular transaction tree of the same block
+		// since such outputs never exist beyond the scope of the view and
+		// therefore do not have any cache entries.
+		//
+		// Further, it is no longer needed in the view either since it's already
+		// spent and thus nothing else in future blocks could possibly spend it.
+		//
+		// Thus, remove the entry from the view and continue to the next one.
+		if entry.isSpentByZeroConf() {
+			// Sanity check zero confirmation spends are also marked as spent.
+			if !entry.IsSpent() {
+				return AssertError(fmt.Sprintf("output %v from view@%s is "+
+					"simultaneously marked as spent by a transaction later in "+
+					"the same block and not spent", outpoint,
+					view.BestHash()))
+			}
+
+			delete(view.entries, outpoint)
+			continue
+		}
+
 		// Mark the spent view entry as modified and spent in the cache and
 		// remove it from the view.
 		//
diff --git a/internal/blockchain/utxocache_test.go b/internal/blockchain/utxocache_test.go
index 95813a8d6e..3e7aecdd18 100644
--- a/internal/blockchain/utxocache_test.go
+++ b/internal/blockchain/utxocache_test.go
@@ -152,14 +152,15 @@ func entry85314() *UtxoEntry {
 // testEntryStates houses a test utxo entry along with various spent, modified,
 // and fresh states for conenient use throughout the tests.
 type testEntryStates struct {
-	unmodified           *UtxoEntry
-	unmodifiedSpent      *UtxoEntry
-	unmodifiedFresh      *UtxoEntry
-	unmodifiedSpentFresh *UtxoEntry
-	modified             *UtxoEntry
-	modifiedSpent        *UtxoEntry
-	modifiedFresh        *UtxoEntry
-	modifiedSpentFresh   *UtxoEntry
+	unmodified              *UtxoEntry
+	unmodifiedSpent         *UtxoEntry
+	unmodifiedFresh         *UtxoEntry
+	unmodifiedSpentFresh    *UtxoEntry
+	modified                *UtxoEntry
+	modifiedSpent           *UtxoEntry
+	modifiedFresh           *UtxoEntry
+	modifiedSpentFresh      *UtxoEntry
+	modifiedSpentByZeroConf *UtxoEntry
 }
 
 // makeEntryStates creates combinations of the passed test utxo entry in various
@@ -181,16 +182,19 @@ func makeEntryStates(entry *UtxoEntry) testEntryStates {
 	modifiedFresh.state |= utxoStateFresh
 	modifiedSpentFresh := modifiedSpent.Clone()
 	modifiedSpentFresh.state |= utxoStateFresh
+	modifiedSpentByZeroConf := modifiedSpent.Clone()
+	modifiedSpentByZeroConf.state |= utxoStateSpentByZeroConf
 
 	return testEntryStates{
-		unmodified:           unmodified,
-		unmodifiedSpent:      unmodifiedSpent,
-		unmodifiedFresh:      unmodifiedFresh,
-		unmodifiedSpentFresh: unmodifiedSpentFresh,
-		modified:             modified,
-		modifiedSpent:        modifiedSpent,
-		modifiedFresh:        modifiedFresh,
-		modifiedSpentFresh:   modifiedSpentFresh,
+		unmodified:              unmodified,
+		unmodifiedSpent:         unmodifiedSpent,
+		unmodifiedFresh:         unmodifiedFresh,
+		unmodifiedSpentFresh:    unmodifiedSpentFresh,
+		modified:                modified,
+		modifiedSpent:           modifiedSpent,
+		modifiedFresh:           modifiedFresh,
+		modifiedSpentFresh:      modifiedSpentFresh,
+		modifiedSpentByZeroConf: modifiedSpentByZeroConf,
 	}
 }
 
@@ -703,6 +707,16 @@ func TestCommit(t *testing.T) {
 		wantCachedEntries map[wire.OutPoint]*UtxoEntry // expected committed cache
 		err               error                        // expected error
 	}{{
+		name: "modified spent by zero conf view entry w/o spent asserts",
+		viewEntries: map[wire.OutPoint]*UtxoEntry{
+			outpoint1200: func() *UtxoEntry {
+				invalidEntry := entry1200.modifiedSpentByZeroConf.Clone()
+				invalidEntry.state &^= utxoStateSpent
+				return invalidEntry
+			}(),
+		},
+		err: AssertError(""),
+	}, {
 		name: "modified spent view entry w/ existing spent cache entry asserts",
 		viewEntries: map[wire.OutPoint]*UtxoEntry{
 			outpoint1200: entry1200.modifiedSpent,
@@ -907,6 +921,18 @@ func TestCommit(t *testing.T) {
 			outpoint1200:  entry1200.modifiedFresh,
 			outpoint85314: entry85314.modified,
 		},
+	}, {
+		name: "view entry spent by zero conf removed and has no effect on cache",
+		viewEntries: map[wire.OutPoint]*UtxoEntry{
+			outpoint299: entry299.modifiedSpentByZeroConf,
+		},
+		cachedEntries: map[wire.OutPoint]*UtxoEntry{
+			outpoint1100: entry1100.unmodified,
+		},
+		wantViewEntries: map[wire.OutPoint]*UtxoEntry{},
+		wantCachedEntries: map[wire.OutPoint]*UtxoEntry{
+			outpoint1100: entry1100.unmodified,
+		},
 	}}
 
 	for _, test := range tests {
diff --git a/internal/blockchain/utxoentry.go b/internal/blockchain/utxoentry.go
index c0a1fadf6b..9c05bb713d 100644
--- a/internal/blockchain/utxoentry.go
+++ b/internal/blockchain/utxoentry.go
@@ -20,19 +20,24 @@ const (
 //
 // The bit representation is:
 //
-//	bit  0    - transaction output has been spent
-//	bit  1    - transaction output has been modified since it was loaded
-//	bit  2    - transaction output is fresh
-//	bits 3-7  - unused
+//	bit  0    - transaction output has been modified since it was loaded
+//	bit  1    - transaction output has been spent
+//	bit  2    - transaction output has been spent by tx later in the same block
+//	bit  3    - transaction output is fresh
+//	bits 4-7  - unused
 type utxoState uint8
 
 const (
-	// utxoStateSpent indicates that a txout is spent.
-	utxoStateSpent utxoState = 1 << iota
-
 	// utxoStateModified indicates that a txout has been modified since it was
 	// loaded.
-	utxoStateModified
+	utxoStateModified utxoState = 1 << iota
+
+	// utxoStateSpent indicates that a txout is spent.
+	utxoStateSpent
+
+	// utxoStateSpentByZeroConf indicates that a txout was spent by another
+	// transaction later in the same block.
+	utxoStateSpentByZeroConf
 
 	// utxoStateFresh indicates that a txout is fresh, which means that it
 	// exists in the utxo cache but does not exist in the underlying database.
@@ -149,6 +154,13 @@ func (entry *UtxoEntry) isFresh() bool {
 	return entry.state&utxoStateFresh == utxoStateFresh
 }
 
+// isSpentByZeroConf returns whether or not the output is marked as spent by a
+// zero confirmation transaction.  In other words, it is an output that was
+// created in a block and later spent by another transaction in the same block.
+func (entry *UtxoEntry) isSpentByZeroConf() bool {
+	return entry.state&utxoStateSpentByZeroConf == utxoStateSpentByZeroConf
+}
+
 // IsCoinBase returns whether or not the output was contained in a coinbase
 // transaction.
 func (entry *UtxoEntry) IsCoinBase() bool {
diff --git a/internal/blockchain/utxoviewpoint.go b/internal/blockchain/utxoviewpoint.go
index 7f4cb54821..63bad2b174 100644
--- a/internal/blockchain/utxoviewpoint.go
+++ b/internal/blockchain/utxoviewpoint.go
@@ -90,7 +90,7 @@ func (view *UtxoViewpoint) addTxOut(outpoint wire.OutPoint, txOut *wire.TxOut,
 
 	// The referenced transaction output should always be marked as unspent and
 	// modified when being added to the view.
-	entry.state &^= utxoStateSpent
+	entry.state &^= utxoStateSpent | utxoStateSpentByZeroConf
 	entry.state |= utxoStateModified
 
 	// Deep copy the script when the script in the entry differs from the one in
@@ -318,19 +318,26 @@ func (view *UtxoViewpoint) connectStakeTransactions(block *dcrutil.Block,
 
 // connectRegularTransaction updates the view by adding all new utxos created by
 // the passed transaction from the regular transaction tree and marking all
-// utxos that the transaction spends as spent.  In addition, when the 'stxos'
-// argument is not nil, it will be updated to append an entry for each spent
-// txout.  An error will be returned if the view does not contain the required
-// utxos.
+// utxos that the transaction spends as spent.  The transaction is added to the
+// provided in-flight tx map which is used to detect and mark utxos earlier in
+// the same regular transaction tree as zero conf spends.
+//
+// When the 'stxos' argument is not nil, it will be updated to append an entry
+// for each spent txout.  An error will be returned if the view does not contain
+// the required utxos.
 func (view *UtxoViewpoint) connectRegularTransaction(tx *dcrutil.Tx,
-	blockHeight int64, blockIndex uint32, stxos *[]spentTxOut,
-	isTreasuryEnabled bool) error {
+	blockHeight int64, blockIndex uint32, inFlightTx map[chainhash.Hash]uint32,
+	stxos *[]spentTxOut, isTreasuryEnabled bool) error {
 
 	// Coinbase transactions don't have any inputs to spend.
 	msgTx := tx.MsgTx()
 	if standalone.IsCoinBaseTx(msgTx, isTreasuryEnabled) {
 		// Add the transaction's outputs as available utxos.
 		view.AddTxOuts(tx, blockHeight, blockIndex, isTreasuryEnabled)
+
+		// Keep track of in-flight transactions in order detect spends of
+		// earlier outputs by transactions later in the same block.
+		inFlightTx[*tx.Hash()] = blockIndex
 		return nil
 	}
 
@@ -355,27 +362,42 @@ func (view *UtxoViewpoint) connectRegularTransaction(tx *dcrutil.Tx,
 		// details have been accessed since spending it might clear the fields
 		// from memory in the future.
 		entry.Spend()
+
+		// Mark txouts that are spent by transaction inputs later in the regular
+		// transaction tree of the same block as zero conf spends.
+		if inFlightIdx, ok := inFlightTx[prevOut.Hash]; ok && blockIndex >
+			inFlightIdx {
+
+			entry.state |= utxoStateSpentByZeroConf
+		}
 	}
 
 	// Add the transaction's outputs as available utxos.
 	view.AddTxOuts(tx, blockHeight, blockIndex, isTreasuryEnabled)
 
+	// Keep track of in-flight transactions in order detect spends of earlier
+	// outputs by transactions later in the same block.
+	inFlightTx[*tx.Hash()] = blockIndex
 	return nil
 }
 
 // connectRegularTransactions updates the view by adding all new utxos created
 // by the transactions in the regular transaction tree of the block and marking
-// all utxos those same transactions spend as spent.  In addition, when the
-// 'stxos' argument is not nil, it will be updated to append an entry for each
-// spent txout.  An error will be returned if the view does not contain the
-// required utxos.
+// all utxos those same transactions spend as spent.  It also marks all earlier
+// utxos spent by transactions later in the tree as zero confirmation spends.
+//
+// When the 'stxos' argument is not nil, it will be updated to append an entry
+// for each spent txout.  An error will be returned if the view does not contain
+// the required utxos.
 func (view *UtxoViewpoint) connectRegularTransactions(block *dcrutil.Block,
 	stxos *[]spentTxOut, isTreasuryEnabled bool) error {
 
 	// Connect all of the transactions in the regular transaction tree.
-	for i, tx := range block.Transactions() {
+	regularTxns := block.Transactions()
+	inFlightTx := make(map[chainhash.Hash]uint32, len(regularTxns))
+	for i, tx := range regularTxns {
 		err := view.connectRegularTransaction(tx, block.Height(), uint32(i),
-			stxos, isTreasuryEnabled)
+			inFlightTx, stxos, isTreasuryEnabled)
 		if err != nil {
 			return err
 		}
@@ -398,11 +420,20 @@ func (view *UtxoViewpoint) disconnectTransactions(block *dcrutil.Block,
 	// disconnecting stake transactions.
 	stxoIdx := len(stxos) - 1
 	transactions := block.Transactions()
+	numSpentRegularOutputs := countSpentRegularOutputs(block)
 	if stakeTree {
-		stxoIdx = len(stxos) - countSpentRegularOutputs(block) - 1
+		stxoIdx = len(stxos) - numSpentRegularOutputs - 1
 		transactions = block.STransactions()
 	}
 
+	// Create a map to keep track of all in-flight spends by the regular
+	// transaction tree in order detect spends of earlier outputs by
+	// transactions later in the same block.
+	var spendsInFlight map[wire.OutPoint]int
+	if !stakeTree {
+		spendsInFlight = make(map[wire.OutPoint]int, numSpentRegularOutputs)
+	}
+
 	for txIdx := len(transactions) - 1; txIdx > -1; txIdx-- {
 		tx := transactions[txIdx]
 		txHash := tx.Hash()
@@ -481,6 +512,14 @@ func (view *UtxoViewpoint) disconnectTransactions(block *dcrutil.Block,
 			}
 
 			entry.Spend()
+
+			// Mark txouts that are spent by transaction inputs later in the
+			// same block as zero conf spends.
+			if inFlightIdx, ok := spendsInFlight[outpoint]; ok &&
+				txIdx < inFlightIdx {
+
+				entry.state |= utxoStateSpentByZeroConf
+			}
 		}
 
 		// Loop backwards through all of the transaction inputs (except for the
@@ -524,8 +563,15 @@ func (view *UtxoViewpoint) disconnectTransactions(block *dcrutil.Block,
 
 			// Mark the existing referenced transaction output as unspent and
 			// modified.
-			entry.state &^= utxoStateSpent
+			entry.state &^= utxoStateSpent | utxoStateSpentByZeroConf
 			entry.state |= utxoStateModified
+
+			// Keep track of all in-flight spends by the regular transaction
+			// tree in order detect spends of earlier outputs by transactions
+			// later in the same block.
+			if !stakeTree {
+				spendsInFlight[txIn.PreviousOutPoint] = txIdx
+			}
 		}
 	}
 
diff --git a/internal/blockchain/validate.go b/internal/blockchain/validate.go
index d4faf9ae7c..9b5262fad4 100644
--- a/internal/blockchain/validate.go
+++ b/internal/blockchain/validate.go
@@ -3471,6 +3471,10 @@ func (b *BlockChain) checkTransactionsAndConnect(inputFees dcrutil.Amount, node
 	// expensive (though still relatively cheap as compared to running the
 	// scripts) checks against all the inputs when the signature operations
 	// are out of bounds.
+	var inFlightRegularTx map[chainhash.Hash]uint32
+	if !stakeTree {
+		inFlightRegularTx = make(map[chainhash.Hash]uint32, len(txs))
+	}
 	totalFees := int64(inputFees) // Stake tx tree carry forward
 	prevHeader := node.parent.Header()
 	var cumulativeSigOps int
@@ -3515,16 +3519,24 @@ func (b *BlockChain) checkTransactionsAndConnect(inputFees dcrutil.Amount, node
 		// and add all of the outputs for this transaction which are not
 		// provably unspendable as available utxos.
 		//
+		// For the regular transaction tree, keep track of in-flight
+		// transactions in order detect spends of earlier outputs by
+		// transactions later in the same block.
+		//
 		// Also, update the passed spent txos slice to contain an entry for each
 		// output the transaction spends.
-		connectTransaction := view.connectRegularTransaction
-		if stakeTree {
-			connectTransaction = view.connectStakeTransaction
-		}
-		err = connectTransaction(tx, node.height, uint32(idx), stxos,
-			isTreasuryEnabled)
-		if err != nil {
-			return err
+		if !stakeTree {
+			err := view.connectRegularTransaction(tx, node.height, uint32(idx),
+				inFlightRegularTx, stxos, isTreasuryEnabled)
+			if err != nil {
+				return err
+			}
+		} else {
+			err := view.connectStakeTransaction(tx, node.height, uint32(idx),
+				stxos, isTreasuryEnabled)
+			if err != nil {
+				return err
+			}
 		}
 	}