Fix a bug with invalidating blocks in new DB and add more sanity checks (#343)

A bug prevented STXO data from being written correctly in the event
that a block became invalidated. This could cause failures on testnet
when a block was invalidated. There was also some debug code and a panic
that were in master that have now been removed. A new test has been
added to ensure that STXO data is being properly written, and more
stringent checks for STXO data validity when added or removing blocks
has been added.

blockchain/chain.go

@@ -1205,12 +1205,6 @@ func (b *BlockChain) connectBlock(node *blockNode, block *dcrutil.Block,
 		// the block that contains all txos spent by it.
 		err = dbPutSpendJournalEntry(dbTx, block.Sha(), stxos)
 		if err != nil {
-			// Attempt to restore TicketDb if this fails.
-			_, _, _, errRemove := b.tmdb.RemoveBlockToHeight(node.height - 1)
-			if errRemove != nil {
-				return errRemove
-			}
-
 			return err
 		}
 
@@ -1535,13 +1529,21 @@ func (b *BlockChain) reorganizeChain(detachNodes, attachNodes *list.List,
 		// journal.
 		var stxos []spentTxOut
 		err = b.db.View(func(dbTx database.Tx) error {
-			stxos, err = dbFetchSpendJournalEntry(dbTx, block, parent, view)
+			stxos, err = dbFetchSpendJournalEntry(dbTx, block, parent)
 			return err
 		})
 		if err != nil {
 			return err
 		}
 
+		// Quick sanity test.
+		if len(stxos) != countSpentOutputs(block, parent) {
+			return AssertError(fmt.Sprintf("retrieved %v stxos when trying to "+
+				"disconnect block %v (height %v), yet counted %v "+
+				"many spent utxos", len(stxos), block.Sha(), block.Height(),
+				countSpentOutputs(block, parent)))
+		}
+
 		// Store the loaded block and spend journal entry for later.
 		detachBlocks = append(detachBlocks, block)
 		detachSpentTxOuts = append(detachSpentTxOuts, stxos)
@@ -1747,13 +1749,22 @@ func (b *BlockChain) forceHeadReorganization(formerBest chainhash.Hash,
 	var stxos []spentTxOut
 	err = b.db.View(func(dbTx database.Tx) error {
 		stxos, err = dbFetchSpendJournalEntry(dbTx, formerBestBlock,
-			commonParentBlock, view)
+			commonParentBlock)
 		return err
 	})
 	if err != nil {
 		return err
 	}
 
+	// Quick sanity test.
+	if len(stxos) != countSpentOutputs(formerBestBlock, commonParentBlock) {
+		return AssertError(fmt.Sprintf("retrieved %v stxos when trying to "+
+			"disconnect block %v (height %v), yet counted %v "+
+			"many spent utxos when trying to force head reorg", len(stxos),
+			formerBestBlock.Sha(), formerBestBlock.Height(),
+			countSpentOutputs(formerBestBlock, commonParentBlock)))
+	}
+
 	err = b.disconnectTransactions(view, formerBestBlock, commonParentBlock,
 		stxos)
 	if err != nil {

blockchain/chainio.go

@@ -388,8 +388,7 @@ func decodeSpentTxOut(serialized []byte, stxo *spentTxOut, amount int64,
 // format comments, this function also requires the transactions that spend the
 // txouts and a utxo view that contains any remaining existing utxos in the
 // transactions referenced by the inputs to the passed transasctions.
-func deserializeSpendJournalEntry(serialized []byte, txns []*wire.MsgTx,
-	view *UtxoViewpoint) ([]spentTxOut, error) {
+func deserializeSpendJournalEntry(serialized []byte, txns []*wire.MsgTx) ([]spentTxOut, error) {
 	// Calculate the total number of stxos.
 	var numStxos int
 	for _, tx := range txns {
@@ -479,7 +478,6 @@ func serializeSpendJournalEntry(stxos []spentTxOut) ([]byte, error) {
 	var size int
 	var sizes []int
 	for i := range stxos {
-		// size += spentTxOutSerializeSize(&stxos[i])
 		sz := spentTxOutSerializeSize(&stxos[i])
 		sizes = append(sizes, sz)
 		size += sz
@@ -509,18 +507,24 @@ func serializeSpendJournalEntry(stxos []spentTxOut) ([]byte, error) {
 // the passed block since that information is required to reconstruct the spent
 // txouts.
 func dbFetchSpendJournalEntry(dbTx database.Tx, block *dcrutil.Block,
-	parent *dcrutil.Block, view *UtxoViewpoint) ([]spentTxOut, error) {
+	parent *dcrutil.Block) ([]spentTxOut, error) {
 	// Exclude the coinbase transaction since it can't spend anything.
 	spendBucket := dbTx.Metadata().Bucket(dbnamespace.SpendJournalBucketName)
 	serialized := spendBucket.Get(block.Sha()[:])
-	blockTxns := append(parent.MsgBlock().Transactions[1:],
-		block.MsgBlock().STransactions...)
+
+	var blockTxns []*wire.MsgTx
+	regularTxTreeValid := dcrutil.IsFlagSet16(block.MsgBlock().Header.VoteBits,
+		dcrutil.BlockValid)
+	if regularTxTreeValid {
+		blockTxns = append(blockTxns, parent.MsgBlock().Transactions[1:]...)
+	}
+	blockTxns = append(blockTxns, block.MsgBlock().STransactions...)
 
 	if len(blockTxns) > 0 && len(serialized) == 0 {
 		return nil, AssertError("missing spend journal data")
 	}
 
-	stxos, err := deserializeSpendJournalEntry(serialized, blockTxns, view)
+	stxos, err := deserializeSpendJournalEntry(serialized, blockTxns)
 	if err != nil {
 		// Ensure any deserialization errors are returned as database
 		// corruption errors.

blockchain/chainio_test.go

@@ -464,7 +464,7 @@ func TestSpendJournalSerialization(t *testing.T) {
 
 		// Deserialize to a spend journal entry.
 		gotEntry, err := deserializeSpendJournalEntry(test.serialized,
-			test.blockTxns, test.utxoView)
+			test.blockTxns)
 		if err != nil {
 			t.Errorf("deserializeSpendJournalEntry #%d (%s) "+
 				"unexpected error: %v", i, test.name, err)
@@ -492,7 +492,6 @@ func TestSpendJournalErrors(t *testing.T) {
 	tests := []struct {
 		name       string
 		blockTxns  []*wire.MsgTx
-		utxoView   *UtxoViewpoint
 		serialized []byte
 		errType    error
 	}{
@@ -511,7 +510,6 @@ func TestSpendJournalErrors(t *testing.T) {
 				}},
 				LockTime: 0,
 			}},
-			utxoView:   NewUtxoViewpoint(),
 			serialized: hexToBytes(""),
 			errType:    AssertError(""),
 		},
@@ -529,7 +527,6 @@ func TestSpendJournalErrors(t *testing.T) {
 				}},
 				LockTime: 0,
 			}},
-			utxoView:   NewUtxoViewpoint(),
 			serialized: hexToBytes("1301320511db93e1dcdb8a016b49840f8c53bc1eb68a382e97b1482ecad7b148a6909a"),
 			errType:    errDeserialize(""),
 		},
@@ -539,7 +536,7 @@ func TestSpendJournalErrors(t *testing.T) {
 		// Ensure the expected error type is returned and the returned
 		// slice is nil.
 		stxos, err := deserializeSpendJournalEntry(test.serialized,
-			test.blockTxns, test.utxoView)
+			test.blockTxns)
 		if reflect.TypeOf(err) != reflect.TypeOf(test.errType) {
 			t.Errorf("deserializeSpendJournalEntry (%s): expected "+
 				"error type does not match - got %T, want %T",

blockchain/common.go

@@ -9,11 +9,48 @@ import (
 	"github.com/decred/dcrd/blockchain/stake"
 	"github.com/decred/dcrd/chaincfg"
 	"github.com/decred/dcrd/chaincfg/chainhash"
+	"github.com/decred/dcrd/database"
 	"github.com/decred/dcrd/txscript"
 	"github.com/decred/dcrd/wire"
 	"github.com/decred/dcrutil"
 )
 
+// DoStxoTest does a test on a simulated blockchain to ensure that the data
+// stored in the STXO buckets is not corrupt.
+func (b *BlockChain) DoStxoTest() error {
+	err := b.db.View(func(dbTx database.Tx) error {
+		for i := int64(2); i <= b.bestNode.height; i++ {
+			block, err := dbFetchBlockByHeight(dbTx, i)
+			if err != nil {
+				return err
+			}
+
+			parent, err := dbFetchBlockByHeight(dbTx, i-1)
+			if err != nil {
+				return err
+			}
+
+			ntx := countSpentOutputs(block, parent)
+			stxos, err := dbFetchSpendJournalEntry(dbTx, block, parent)
+			if err != nil {
+				return err
+			}
+
+			if int(ntx) != len(stxos) {
+				fmt.Printf("bad number of stxos calculated at height %v, got %v expected %v\n",
+					i, len(stxos), int(ntx))
+			}
+		}
+
+		return nil
+	})
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
 // DebugBlockHeaderString dumps a verbose message containing information about
 // the block header of a block.
 func DebugBlockHeaderString(chainParams *chaincfg.Params,

blockchain/ticketlookup.go

@@ -621,12 +621,21 @@ func (b *BlockChain) fetchTicketStore(node *blockNode) (TicketStore, error) {
 		// journal.
 		var stxos []spentTxOut
 		err = b.db.View(func(dbTx database.Tx) error {
-			stxos, err = dbFetchSpendJournalEntry(dbTx, block, parent, view)
+			stxos, err = dbFetchSpendJournalEntry(dbTx, block, parent)
 			return err
 		})
 		if err != nil {
 			return nil, err
 		}
+
+		// Quick sanity test.
+		if len(stxos) != countSpentOutputs(block, parent) {
+			return nil, AssertError(fmt.Sprintf("retrieved %v stxos when "+
+				"trying to disconnect block %v (height %v), yet counted %v "+
+				"many spent utxos when fetching ticket store", len(stxos),
+				block.Sha(), block.Height(), countSpentOutputs(block, parent)))
+		}
+
 		err = b.disconnectTransactions(view, block, parent, stxos)
 		if err != nil {
 			return nil, err

blockchain/utxoviewpoint.go

@@ -496,8 +496,6 @@ func (b *BlockChain) disconnectTransactions(view *UtxoViewpoint,
 	// Loop backwards through all transactions so everything is unspent in
 	// reverse order.  This is necessary since transactions later in a block
 	// can spend from previous ones.
-	// debug TODO remove
-	//stxoIdxParent, stxoIdxCurrent := countSpentOutputsPerTree(block, parent)
 	regularTxTreeValid := dcrutil.IsFlagSet16(block.MsgBlock().Header.VoteBits,
 		dcrutil.BlockValid)
 	thisNodeStakeViewpoint := ViewpointPrevInvalidStake
@@ -904,27 +902,6 @@ func (view *UtxoViewpoint) fetchInputUtxos(db database.DB,
 	block, parent *dcrutil.Block) error {
 	viewpoint := view.StakeViewpoint()
 
-	// If we need the previous block, grab it.
-	/*
-		var parent *dcrutil.Block
-		if viewpoint == ViewpointPrevValidInitial ||
-			viewpoint == ViewpointPrevValidStake {
-			prevBlock := block.MsgBlock().Header.PrevBlock
-			err := db.View(func(dbTx database.Tx) error {
-				var err error
-				parent, err = dbFetchBlockByHash(dbTx, &prevBlock)
-				if err != nil {
-					return err
-				}
-
-				return nil
-			})
-			if err != nil {
-				return err
-			}
-		}
-	*/
-
 	// Build a map of in-flight transactions because some of the inputs in
 	// this block could be referencing other transactions earlier in this
 	// block which are not yet in the chain.
@@ -992,8 +969,8 @@ func (view *UtxoViewpoint) fetchInputUtxos(db database.DB,
 		// in-flight in relation to the regular tx tree or to other tx in
 		// the stake tx tree, so don't do any of those expensive checks and
 		// just append it to the tx slice.
-		stransactions := block.STransactions()
-		for _, tx := range stransactions {
+		sTransactions := block.STransactions()
+		for _, tx := range sTransactions {
 			isSSGen, _ := stake.IsSSGen(tx)
 
 			for i, txIn := range tx.MsgTx().TxIn {

blockchain/validate.go

@@ -2446,7 +2446,7 @@ func (b *BlockChain) CheckConnectBlock(block *dcrutil.Block) error {
 	view := NewUtxoViewpoint()
 	view.SetBestHash(b.bestNode.hash)
 	view.SetStakeViewpoint(ViewpointPrevValidInitial)
-
+	var stxos []spentTxOut
 	for e := detachNodes.Front(); e != nil; e = e.Next() {
 		n := e.Value.(*blockNode)
 		block, err := b.getBlockFromHash(n.hash)
@@ -2461,9 +2461,8 @@ func (b *BlockChain) CheckConnectBlock(block *dcrutil.Block) error {
 
 		// Load all of the spent txos for the block from the spend
 		// journal.
-		var stxos []spentTxOut
 		err = b.db.View(func(dbTx database.Tx) error {
-			stxos, err = dbFetchSpendJournalEntry(dbTx, block, parent, view)
+			stxos, err = dbFetchSpendJournalEntry(dbTx, block, parent)
 			return err
 		})
 		if err != nil {
@@ -2503,7 +2502,6 @@ func (b *BlockChain) CheckConnectBlock(block *dcrutil.Block) error {
 			return err
 		}
 
-		var stxos []spentTxOut
 		err = b.connectTransactions(view, block, parent, &stxos)
 		if err != nil {
 			return err
@@ -2511,5 +2509,5 @@ func (b *BlockChain) CheckConnectBlock(block *dcrutil.Block) error {
 	}
 
 	view.SetBestHash(&parentHash)
-	return b.checkConnectBlock(newNode, block, view, nil)
+	return b.checkConnectBlock(newNode, block, view, &stxos)
 }

blockchain/validate_test.go

@@ -1968,6 +1968,67 @@ func TestBlockValidationRules(t *testing.T) {
 	}
 }
 
+// TestBlockchainSpendJournal tests for whether or not the spend journal is being
+// written to disk correctly on a live blockchain.
+func TestBlockchainSpendJournal(t *testing.T) {
+	// Create a new database and chain instance to run tests against.
+	chain, teardownFunc, err := chainSetup("reorgunittest",
+		simNetParams)
+	if err != nil {
+		t.Errorf("Failed to setup chain instance: %v", err)
+		return
+	}
+	defer teardownFunc()
+
+	// The genesis block should fail to connect since it's already
+	// inserted.
+	genesisBlock := simNetParams.GenesisBlock
+	err = chain.CheckConnectBlock(dcrutil.NewBlock(genesisBlock))
+	if err == nil {
+		t.Errorf("CheckConnectBlock: Did not receive expected error")
+	}
+
+	// Load up the rest of the blocks up to HEAD.
+	filename := filepath.Join("testdata/", "reorgto179.bz2")
+	fi, err := os.Open(filename)
+	bcStream := bzip2.NewReader(fi)
+	defer fi.Close()
+
+	// Create a buffer of the read file
+	bcBuf := new(bytes.Buffer)
+	bcBuf.ReadFrom(bcStream)
+
+	// Create decoder from the buffer and a map to store the data
+	bcDecoder := gob.NewDecoder(bcBuf)
+	blockChain := make(map[int64][]byte)
+
+	// Decode the blockchain into the map
+	if err := bcDecoder.Decode(&blockChain); err != nil {
+		t.Errorf("error decoding test blockchain: %v", err.Error())
+	}
+
+	// Load up the short chain
+	timeSource := blockchain.NewMedianTime()
+	finalIdx1 := 179
+	for i := 1; i < finalIdx1+1; i++ {
+		bl, err := dcrutil.NewBlockFromBytes(blockChain[int64(i)])
+		if err != nil {
+			t.Fatalf("NewBlockFromBytes error: %v", err.Error())
+		}
+		bl.SetHeight(int64(i))
+
+		_, _, err = chain.ProcessBlock(bl, timeSource, blockchain.BFNone)
+		if err != nil {
+			t.Fatalf("ProcessBlock error at height %v: %v", i, err.Error())
+		}
+	}
+
+	err = chain.DoStxoTest()
+	if err != nil {
+		t.Errorf(err.Error())
+	}
+}
+
 // simNetPowLimit is the highest proof of work value a Decred block
 // can have for the simulation test network.  It is the value 2^255 - 1.
 var simNetPowLimit = new(big.Int).Sub(new(big.Int).Lsh(bigOne, 255), bigOne)

blockmanager.go

@@ -1220,7 +1220,6 @@ func (b *blockManager) handleBlockMsg(bmsg *blockMsg) {
 	// handling, etc.
 	onMainChain, isOrphan, err := b.chain.ProcessBlock(bmsg.block,
 		b.server.timeSource, behaviorFlags)
-
 	if err != nil {
 		// When the error is a rule error, it means the block was simply
 		// rejected as opposed to something actually going wrong, so log
@@ -1235,7 +1234,7 @@ func (b *blockManager) handleBlockMsg(bmsg *blockMsg) {
 		}
 		if dbErr, ok := err.(database.Error); ok && dbErr.ErrorCode ==
 			database.ErrCorruption {
-			panic(dbErr)
+			bmgrLog.Errorf("Critical failure: %v", dbErr.Error())
 		}
 
 		// Convert the error into an appropriate reject message and