From 16f6dab4021482b82ef4960144c2beaba0154bfa Mon Sep 17 00:00:00 2001 From: C Jepson Date: Tue, 13 Sep 2016 11:27:28 -0400 Subject: [PATCH] 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 | 27 +++++++++++----- blockchain/chainio.go | 18 ++++++----- blockchain/chainio_test.go | 7 ++--- blockchain/common.go | 37 ++++++++++++++++++++++ blockchain/ticketlookup.go | 11 ++++++- blockchain/utxoviewpoint.go | 27 ++-------------- blockchain/validate.go | 8 ++--- blockchain/validate_test.go | 61 +++++++++++++++++++++++++++++++++++++ blockmanager.go | 3 +- 9 files changed, 146 insertions(+), 53 deletions(-) diff --git a/blockchain/chain.go b/blockchain/chain.go index 777941b0..b41f263c 100644 --- a/blockchain/chain.go +++ b/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 { diff --git a/blockchain/chainio.go b/blockchain/chainio.go index 92504409..7f2eaadc 100644 --- a/blockchain/chainio.go +++ b/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. diff --git a/blockchain/chainio_test.go b/blockchain/chainio_test.go index 991a4575..894eeabc 100644 --- a/blockchain/chainio_test.go +++ b/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", diff --git a/blockchain/common.go b/blockchain/common.go index f7a94079..f4e1702b 100644 --- a/blockchain/common.go +++ b/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, diff --git a/blockchain/ticketlookup.go b/blockchain/ticketlookup.go index e24317b3..bd98e6d0 100644 --- a/blockchain/ticketlookup.go +++ b/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 diff --git a/blockchain/utxoviewpoint.go b/blockchain/utxoviewpoint.go index 799e006e..e7b37af8 100644 --- a/blockchain/utxoviewpoint.go +++ b/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 { diff --git a/blockchain/validate.go b/blockchain/validate.go index 9709008d..7772ed37 100644 --- a/blockchain/validate.go +++ b/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) } diff --git a/blockchain/validate_test.go b/blockchain/validate_test.go index a323bbb0..9dd597c3 100644 --- a/blockchain/validate_test.go +++ b/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) diff --git a/blockmanager.go b/blockmanager.go index 0cd58cc7..310a0506 100644 --- a/blockmanager.go +++ b/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