Merge pull request #2122 from kcalvinalvin/2024-02-15-dont-rely-on-height-fetching

blockchain: don't rely on BlockHeightByHash for prune height calculations

blockchain/utxocache.go

@@ -617,6 +617,7 @@ func (b *BlockChain) InitConsistentState(tip *blockNode, interrupt <-chan struct
 
 		// Set the last flush hash as it's the default value of 0s.
 		s.lastFlushHash = tip.hash
+		s.lastFlushTime = time.Now()
 
 		return err
 	}
@@ -725,22 +726,35 @@ func (b *BlockChain) InitConsistentState(tip *blockNode, interrupt <-chan struct
 // Example: if the last flush hash was at height 100 and one of the deleted blocks was at
 // height 98, this function will return true.
 func (b *BlockChain) flushNeededAfterPrune(deletedBlockHashes []chainhash.Hash) (bool, error) {
-	lastFlushHeight, err := b.BlockHeightByHash(&b.utxoCache.lastFlushHash)
-	if err != nil {
-		return false, err
+	node := b.index.LookupNode(&b.utxoCache.lastFlushHash)
+	if node == nil {
+		// If we couldn't find the node where we last flushed at, have the utxo cache
+		// flush to be safe and that will set the last flush hash again.
+		//
+		// This realistically should never happen as nodes are never deleted from
+		// the block index.  This happening likely means that there's a hardware
+		// error which is something we can't recover from.  The best that we can
+		// do here is to just force a flush and hope that the newly set
+		// lastFlushHash doesn't error.
+		return true, nil
 	}
 
+	lastFlushHeight := node.Height()
+
 	// Loop through all the block hashes and find out what the highest block height
 	// among the deleted hashes is.
 	highestDeletedHeight := int32(-1)
 	for _, deletedBlockHash := range deletedBlockHashes {
-		height, err := b.BlockHeightByHash(&deletedBlockHash)
-		if err != nil {
-			return false, err
+		node := b.index.LookupNode(&deletedBlockHash)
+		if node == nil {
+			// If we couldn't find this node, just skip it and try the next
+			// deleted hash.  This might be a corruption in the database
+			// but there's nothing we can do here to address it except for
+			// moving onto the next block.
+			continue
 		}
-
-		if height > highestDeletedHeight {
-			highestDeletedHeight = height
+		if node.height > highestDeletedHeight {
+			highestDeletedHeight = node.height
 		}
 	}
 

blockchain/utxocache_test.go

@@ -729,18 +729,33 @@ func TestFlushOnPrune(t *testing.T) {
 	}
 
 	syncBlocks := func() {
+		// Modify block 1 to be a different hash.  This is to artificially
+		// create a stale branch in the chain.
+		staleMsgBlock := blocks[1].MsgBlock().Copy()
+		staleMsgBlock.Header.Nonce = 0
+		staleBlock := btcutil.NewBlock(staleMsgBlock)
+
+		// Add the stale block here to create a chain view like so. The
+		// block will be the main chain at first but become stale as we
+		// keep adding blocks.  BFNoPoWCheck is given as the pow check will
+		// fail.
+		//
+		// (genesis block) -> 1 -> 2 -> 3 -> ...
+		//                \-> 1a
+		_, _, err = chain.ProcessBlock(staleBlock, BFNoPoWCheck)
+		if err != nil {
+			t.Fatal(err)
+		}
+
 		for i, block := range blocks {
 			if i == 0 {
 				// Skip the genesis block.
 				continue
 			}
-			isMainChain, _, err := chain.ProcessBlock(block, BFNone)
+			_, _, err = chain.ProcessBlock(block, BFNone)
 			if err != nil {
-				t.Fatal(err)
-			}
-
-			if !isMainChain {
-				t.Fatalf("expected block %s to be on the main chain", block.Hash())
+				t.Fatalf("Failed to process block %v(%v). %v",
+					block.Hash().String(), block.Height(), err)
 			}
 		}
 	}
@@ -749,36 +764,40 @@ func TestFlushOnPrune(t *testing.T) {
 	ffldb.TstRunWithMaxBlockFileSize(chain.db, maxBlockFileSize, syncBlocks)
 
 	// Function that errors out if the block that should exist doesn't exist.
-	shouldExist := func(dbTx database.Tx, blockHash *chainhash.Hash) {
+	shouldExist := func(dbTx database.Tx, blockHash *chainhash.Hash) error {
 		bytes, err := dbTx.FetchBlock(blockHash)
 		if err != nil {
-			t.Fatal(err)
+			return err
 		}
 		block, err := btcutil.NewBlockFromBytes(bytes)
 		if err != nil {
-			t.Fatalf("didn't find block %v. %v", blockHash, err)
+			return fmt.Errorf("didn't find block %v. %v", blockHash, err)
 		}
 
 		if !block.Hash().IsEqual(blockHash) {
-			t.Fatalf("expected to find block %v but got %v",
+			return fmt.Errorf("expected to find block %v but got %v",
 				blockHash, block.Hash())
 		}
+
+		return nil
 	}
 
 	// Function that errors out if the block that shouldn't exist exists.
-	shouldNotExist := func(dbTx database.Tx, blockHash *chainhash.Hash) {
+	shouldNotExist := func(dbTx database.Tx, blockHash *chainhash.Hash) error {
 		bytes, err := dbTx.FetchBlock(chaincfg.MainNetParams.GenesisHash)
 		if err == nil {
-			t.Fatalf("expected block %s to be pruned", blockHash)
+			return fmt.Errorf("expected block %s to be pruned", blockHash.String())
 		}
 		if len(bytes) != 0 {
-			t.Fatalf("expected block %s to be pruned but got %v",
+			return fmt.Errorf("expected block %s to be pruned but got %v",
 				blockHash, bytes)
 		}
+
+		return nil
 	}
 
 	// The below code checks that the correct blocks were pruned.
-	chain.db.View(func(dbTx database.Tx) error {
+	err = chain.db.View(func(dbTx database.Tx) error {
 		exist := false
 		for _, block := range blocks {
 			// Blocks up to the last flush hash should not exist.
@@ -789,15 +808,23 @@ func TestFlushOnPrune(t *testing.T) {
 			}
 
 			if exist {
-				shouldExist(dbTx, block.Hash())
+				err = shouldExist(dbTx, block.Hash())
+				if err != nil {
+					return err
+				}
 			} else {
-				shouldNotExist(dbTx, block.Hash())
+				err = shouldNotExist(dbTx, block.Hash())
+				if err != nil {
+					return err
+				}
 			}
-
 		}
 
 		return nil
 	})
+	if err != nil {
+		t.Fatal(err)
+	}
 }
 
 func TestInitConsistentState(t *testing.T) {