Merge pull request #150 from kcalvinalvin/2024-03-05-no-utxocache-loa…

…ding-on-reorgs

blockchain, blockchain_test, fullblocktests: fix inconsistent utxocache bug during reorgs

blockchain/chain.go

@@ -851,6 +851,17 @@ func (b *BlockChain) disconnectBlock(node *blockNode, block *btcutil.Block, view
 			return err
 		}
 
+		if b.utxoCache != nil {
+			// Flush the cache on every disconnect. Since the code for
+			// reorganization modifies the database directly, the cache
+			// will be left in an inconsistent state if we don't flush it
+			// prior to the dbPutUtxoView that happends below.
+			err = b.utxoCache.flush(dbTx, FlushRequired, state)
+			if err != nil {
+				return err
+			}
+		}
+
 		// Update the utxo set using the state of the utxo view.  This
 		// entails restoring all of the utxos spent and removing the new
 		// ones created by the block.
@@ -944,18 +955,6 @@ func (b *BlockChain) reorganizeChain(detachNodes, attachNodes *list.List) error
 		return nil
 	}
 
-	// Utreexo node doesn't have a utxo cache. Only flush if we're not a utreexo node.
-	if b.utreexoView == nil {
-		// The rest of the reorg depends on all STXOs already being in the database
-		// so we flush before reorg.
-		err := b.db.Update(func(dbTx database.Tx) error {
-			return b.utxoCache.flush(dbTx, FlushRequired, b.BestSnapshot())
-		})
-		if err != nil {
-			return err
-		}
-	}
-
 	// Track the old and new best chains heads.
 	tip := b.bestChain.Tip()
 	oldBest := tip
@@ -986,7 +985,7 @@ func (b *BlockChain) reorganizeChain(detachNodes, attachNodes *list.List) error
 		if b.utreexoView == nil {
 			// Load all of the utxos referenced by the block that aren't
 			// already in the view.
-			err := view.fetchInputUtxos(b.db, nil, block)
+			err := view.fetchInputUtxos(b.utxoCache, block)
 			if err != nil {
 				return err
 			}
@@ -1064,7 +1063,7 @@ func (b *BlockChain) reorganizeChain(detachNodes, attachNodes *list.List) error
 		if b.utreexoView == nil {
 			// Load all of the utxos referenced by the block that aren't
 			// already in the view.
-			err := view.fetchInputUtxos(b.db, nil, block)
+			err := view.fetchInputUtxos(b.utxoCache, block)
 			if err != nil {
 				return err
 			}
@@ -1100,17 +1099,6 @@ func (b *BlockChain) reorganizeChain(detachNodes, attachNodes *list.List) error
 		}
 	}
 
-	if b.utreexoView == nil {
-		// We call the flush at the end to update the last flush hash to the new
-		// best tip.
-		err = b.db.Update(func(dbTx database.Tx) error {
-			return b.utxoCache.flush(dbTx, FlushRequired, b.BestSnapshot())
-		})
-		if err != nil {
-			return err
-		}
-	}
-
 	// Log the point where the chain forked and old and new best chain
 	// heads.
 	if forkNode != nil {
@@ -1141,17 +1129,6 @@ func (b *BlockChain) verifyReorganizationValidity(detachNodes, attachNodes *list
 		return nil, nil, nil, nil
 	}
 
-	if b.utreexoView == nil {
-		// Flush the cache before checking the validity. This is because the reorganization code
-		// depends on the assumption that the cache is flushed.
-		err := b.db.Update(func(dbTx database.Tx) error {
-			return b.utxoCache.flush(dbTx, FlushRequired, b.BestSnapshot())
-		})
-		if err != nil {
-			return nil, nil, nil, err
-		}
-	}
-
 	// verifyReorganizationValidity will modify the best chain and the utreexo state
 	// if the node is a utreexo node. Since verifyReorganizationValidity is supposed
 	// to not mutate the chain, we need to revert whatever changes were applied in
@@ -1267,7 +1244,7 @@ func (b *BlockChain) verifyReorganizationValidity(detachNodes, attachNodes *list
 		} else {
 			// Load all of the utxos referenced by the block that aren't
 			// already in the view.
-			err = view.fetchInputUtxos(b.db, nil, block)
+			err = view.fetchInputUtxos(b.utxoCache, block)
 			if err != nil {
 				return nil, nil, nil, err
 			}
@@ -1345,7 +1322,7 @@ func (b *BlockChain) verifyReorganizationValidity(detachNodes, attachNodes *list
 			// the utxo cache.
 			// If we are a utreexo node, we fetch them from the block.
 			if b.utreexoView == nil {
-				err = view.fetchInputUtxos(b.db, nil, block)
+				err = view.fetchInputUtxos(b.utxoCache, block)
 				if err != nil {
 					return nil, nil, nil, err
 				}

blockchain/fullblocks_test.go

@@ -146,6 +146,70 @@ func TestFullBlocks(t *testing.T) {
 	}
 	defer teardownFunc()
 
+	testBlockDisconnectExpectUTXO := func(item fullblocktests.BlockDisconnectExpectUTXO) {
+		expectedCallBack := func(notification *blockchain.Notification) {
+			switch notification.Type {
+
+			case blockchain.NTBlockDisconnected:
+				block, ok := notification.Data.(*btcutil.Block)
+				if !ok {
+					t.Fatalf("expected a block")
+				}
+
+				// Return early if the block we get isn't the relevant
+				// block.
+				if !block.Hash().IsEqual(&item.BlockHash) {
+					return
+				}
+
+				entry, err := chain.FetchUtxoEntry(item.OutPoint)
+				if err != nil {
+					t.Fatal(err)
+				}
+
+				if entry == nil || entry.IsSpent() {
+					t.Logf("expected utxo %v to exist but it's "+
+						"nil or spent\n", item.OutPoint.String())
+					t.Fatalf("expected utxo %v to exist but it's "+
+						"nil or spent", item.OutPoint.String())
+				}
+			}
+		}
+		unexpectedCallBack := func(notification *blockchain.Notification) {
+			switch notification.Type {
+			case blockchain.NTBlockDisconnected:
+				block, ok := notification.Data.(*btcutil.Block)
+				if !ok {
+					t.Fatalf("expected a block")
+				}
+
+				// Return early if the block we get isn't the relevant
+				// block.
+				if !block.Hash().IsEqual(&item.BlockHash) {
+					return
+				}
+
+				entry, err := chain.FetchUtxoEntry(item.OutPoint)
+				if err != nil {
+					t.Fatal(err)
+				}
+
+				if entry != nil && !entry.IsSpent() {
+					t.Logf("unexpected utxo %v to exist but it's "+
+						"not nil and not spent", item.OutPoint.String())
+					t.Fatalf("unexpected utxo %v exists but it's "+
+						"not nil and not spent\n", item.OutPoint.String())
+				}
+			}
+		}
+
+		if item.Expected {
+			chain.Subscribe(expectedCallBack)
+		} else {
+			chain.Subscribe(unexpectedCallBack)
+		}
+	}
+
 	// testAcceptedBlock attempts to process the block in the provided test
 	// instance and ensures that it was accepted according to the flags
 	// specified in the test.
@@ -300,6 +364,8 @@ func TestFullBlocks(t *testing.T) {
 				testOrphanOrRejectedBlock(item)
 			case fullblocktests.ExpectedTip:
 				testExpectedTip(item)
+			case fullblocktests.BlockDisconnectExpectUTXO:
+				testBlockDisconnectExpectUTXO(item)
 			default:
 				t.Fatalf("test #%d, item #%d is not one of "+
 					"the supported test instance types -- "+

blockchain/fullblocktests/generate.go

@@ -186,6 +186,21 @@ type RejectedNonCanonicalBlock struct {
 // This implements the TestInstance interface.
 func (b RejectedNonCanonicalBlock) FullBlockTestInstance() {}
 
+// BlockDisconnectExpectUTXO defines a test instance that tests an utxo to exist or not
+// exist after a specified block has been disconnected.
+type BlockDisconnectExpectUTXO struct {
+	Name      string
+	Expected  bool
+	BlockHash chainhash.Hash
+	OutPoint  wire.OutPoint
+}
+
+// FullBlockTestInstance only exists to allow BlockDisconnectExpectUTXO to be treated as
+// a TestInstance.
+//
+// This implements the TestInstance interface.
+func (b BlockDisconnectExpectUTXO) FullBlockTestInstance() {}
+
 // spendableOut represents a transaction output that is spendable along with
 // additional metadata such as the block its in and how much it pays.
 type spendableOut struct {
@@ -925,6 +940,9 @@ func Generate(includeLargeReorg bool) (tests [][]TestInstance, err error) {
 	//
 	// orphanedOrRejected creates and appends a single orphanOrRejectBlock
 	// test instance for the current tip.
+	//
+	// blockDisconnectExpectUTXO creates and appends a BlockDisconnectExpectUTXO test
+	// instance with the passed in values.
 	accepted := func() {
 		tests = append(tests, []TestInstance{
 			acceptBlock(g.tipName, g.tip, true, false),
@@ -951,6 +969,12 @@ func Generate(includeLargeReorg bool) (tests [][]TestInstance, err error) {
 			orphanOrRejectBlock(g.tipName, g.tip),
 		})
 	}
+	blockDisconnectExpectUTXO := func(name string, expected bool, op wire.OutPoint,
+		hash chainhash.Hash) {
+		tests = append(tests, []TestInstance{
+			BlockDisconnectExpectUTXO{name, expected, hash, op},
+		})
+	}
 
 	// Checking that duplicate blocks are not allowed
 	tests = append(tests, []TestInstance{rejectBlock("genesis", g.tip, blockchain.ErrDuplicateBlock)})
@@ -2093,6 +2117,55 @@ func Generate(includeLargeReorg bool) (tests [][]TestInstance, err error) {
 	}
 	accepted()
 
+	// Create a chain where the utxo created in b82a is spent in b83a.
+	//
+	//   b81() -> b82a(28) -> b83a(b82.tx[1].out[0])
+	//
+	g.nextBlock("b82a", outs[28])
+	accepted()
+
+	b82aTx1Out0 := makeSpendableOut(g.tip, 1, 0)
+	g.nextBlock("b83a", &b82aTx1Out0)
+	accepted()
+
+	// Now we'll build a side-chain where we don't spend any of the outputs.
+	//
+	//   b81() -> b82a(28) -> b83a(b82.tx[1].out[0])
+	//        \-> b82()    -> b83()
+	//
+	g.setTip("b81")
+	g.nextBlock("b82", nil)
+	acceptedToSideChainWithExpectedTip("b83a")
+
+	g.nextBlock("b83", nil)
+	acceptedToSideChainWithExpectedTip("b83a")
+
+	// At this point b83a is still the tip.  When we add block 84, the tip
+	// will change. Pre-load up the expected utxos test before the reorganization.
+	//
+	// We expect b82a output to now be a utxo since b83a was spending it and it was
+	// removed from the main chain.
+	blockDisconnectExpectUTXO("b82aTx1Out0",
+		true, b82aTx1Out0.prevOut, g.blocksByName["b83a"].BlockHash())
+
+	// We expect the output from b82 to not exist once b82a itself has been removed
+	// from the main chain.
+	blockDisconnectExpectUTXO("b82aTx1Out0",
+		false, b82aTx1Out0.prevOut, g.blocksByName["b82a"].BlockHash())
+
+	// The output that was being spent in b82a should exist after the removal of
+	// b82a.
+	blockDisconnectExpectUTXO("outs[28]",
+		true, outs[28].prevOut, g.blocksByName["b82a"].BlockHash())
+
+	// Create block 84 and reorg out the sidechain with b83a as the tip.
+	//
+	//   b81() -> b82a(28) -> b83a(b82.tx[1].out[0])
+	//        \-> b82()    -> b83()                  -> b84()
+	//
+	g.nextBlock("b84", nil)
+	accepted()
+
 	// ---------------------------------------------------------------------
 	// Large block re-org test.
 	// ---------------------------------------------------------------------
@@ -2103,8 +2176,8 @@ func Generate(includeLargeReorg bool) (tests [][]TestInstance, err error) {
 
 	// Ensure the tip the re-org test builds on is the best chain tip.
 	//
-	//   ... -> b81(27) -> ...
-	g.setTip("b81")
+	//   ... -> b84() -> ...
+	g.setTip("b84")
 
 	// Collect all of the spendable coinbase outputs from the previous
 	// collection point up to the current tip.

blockchain/utxoviewpoint.go

@@ -519,41 +519,6 @@ func (view *UtxoViewpoint) commit() {
 	}
 }
 
-// fetchUtxosMain fetches unspent transaction output data about the provided
-// set of outpoints from the point of view of the end of the main chain at the
-// time of the call.
-//
-// Upon completion of this function, the view will contain an entry for each
-// requested outpoint.  Spent outputs, or those which otherwise don't exist,
-// will result in a nil entry in the view.
-func (view *UtxoViewpoint) fetchUtxosMain(db database.DB, outpoints []wire.OutPoint) error {
-	// Nothing to do if there are no requested outputs.
-	if len(outpoints) == 0 {
-		return nil
-	}
-
-	// Load the requested set of unspent transaction outputs from the point
-	// of view of the end of the main chain.
-	//
-	// NOTE: Missing entries are not considered an error here and instead
-	// will result in nil entries in the view.  This is intentionally done
-	// so other code can use the presence of an entry in the store as a way
-	// to unnecessarily avoid attempting to reload it from the database.
-	return db.View(func(dbTx database.Tx) error {
-		utxoBucket := dbTx.Metadata().Bucket(utxoSetBucketName)
-		for i := range outpoints {
-			entry, err := dbFetchUtxoEntry(dbTx, utxoBucket, outpoints[i])
-			if err != nil {
-				return err
-			}
-
-			view.entries[outpoints[i]] = entry
-		}
-
-		return nil
-	})
-}
-
 // fetchUtxosFromCache fetches unspent transaction output data about the provided
 // set of outpoints from the point of view of the end of the main chain at the
 // time of the call.  It attempts to fetch them from the cache and whatever entries
@@ -666,15 +631,12 @@ func (view *UtxoViewpoint) findInputsToFetch(block *btcutil.Block) []wire.OutPoi
 
 // fetchInputUtxos loads the unspent transaction outputs for the inputs
 // referenced by the transactions in the given block into the view from the
-// database or the cache as needed.  In particular, referenced entries that
-// are earlier in the block are added to the view and entries that are already
-// in the view are not modified.
-func (view *UtxoViewpoint) fetchInputUtxos(db database.DB, cache *utxoCache, block *btcutil.Block) error {
-	if cache != nil {
-		return view.fetchUtxosFromCache(cache, view.findInputsToFetch(block))
-	}
+// cache as needed.  In particular, referenced entries that are earlier in
+// the block are added to the view and entries that are already in the view
+// are not modified.
+func (view *UtxoViewpoint) fetchInputUtxos(cache *utxoCache, block *btcutil.Block) error {
 	// Request the input utxos from the cache.
-	return view.fetchUtxosMain(db, view.findInputsToFetch(block))
+	return view.fetchUtxosFromCache(cache, view.findInputsToFetch(block))
 }
 
 // UBlockToUtxoView converts the UData from a block into a UtxoViewpoint. This

blockchain/validate.go

@@ -1085,7 +1085,7 @@ func (b *BlockChain) checkConnectBlock(node *blockNode, block *btcutil.Block,
 			return err
 		}
 	} else {
-		err := view.fetchInputUtxos(nil, b.utxoCache, block)
+		err := view.fetchInputUtxos(b.utxoCache, block)
 		if err != nil {
 			return err
 		}