From 16dc2cf2d0231af4015546c6ef91d7ea3710a6f8 Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Mon, 7 Jul 2014 09:50:50 -0500 Subject: [PATCH] Add errors to all interface methods. This commit adds error returns to all of the Db interface methods except for FetchTxByShaList and FetchUnSpentTxByShaList since they expose the errors on each individual transaction. It also updates all tests and code for both the leveldb and memdb drivers for the changes. Closes #5. ok @drahn --- db.go | 20 +++++++++++++++----- interface_test.go | 17 +++++++++++++++-- ldb/block.go | 23 ++++++++++------------- ldb/boundary_test.go | 10 ++++++++-- ldb/dup_test.go | 12 ++++++++++-- ldb/insertremove_test.go | 13 ++++++++++--- ldb/leveldb.go | 15 ++++++++------- ldb/operational_test.go | 32 ++++++++++++++++++++++++++------ ldb/tx.go | 16 ++++++++-------- memdb/memdb.go | 35 +++++++++++++++++++---------------- memdb/memdb_test.go | 29 ++++++++++++++++++----------- 11 files changed, 147 insertions(+), 75 deletions(-) diff --git a/db.go b/db.go index adfbb405..157f24ed 100644 --- a/db.go +++ b/db.go @@ -30,7 +30,7 @@ const AllShas = int64(^uint64(0) >> 1) // used to add a new backend data storage method. type Db interface { // Close cleanly shuts down the database and syncs all data. - Close() + Close() (err error) // DropAfterBlockBySha will remove any blocks from the database after // the given block. It terminates any existing transaction and performs @@ -40,7 +40,7 @@ type Db interface { // ExistsSha returns whether or not the given block hash is present in // the database. - ExistsSha(sha *btcwire.ShaHash) (exists bool) + ExistsSha(sha *btcwire.ShaHash) (exists bool, err error) // FetchBlockBySha returns a btcutil Block. The implementation may // cache the underlying data if desired. @@ -65,7 +65,7 @@ type Db interface { // ExistsTxSha returns whether or not the given tx hash is present in // the database - ExistsTxSha(sha *btcwire.ShaHash) (exists bool) + ExistsTxSha(sha *btcwire.ShaHash) (exists bool, err error) // FetchTxBySha returns some data for the given transaction hash. The // implementation may cache the underlying data if desired. @@ -75,12 +75,22 @@ type Db interface { // hashes. The implementation may cache the underlying data if desired. // This differs from FetchUnSpentTxByShaList in that it will return // the most recent known Tx, if it is fully spent or not. + // + // NOTE: This function does not return an error directly since it MUST + // return at least one TxListReply instance for each requested + // transaction. Each TxListReply instance then contains an Err field + // which can be used to detect errors. FetchTxByShaList(txShaList []*btcwire.ShaHash) []*TxListReply // FetchUnSpentTxByShaList returns a TxListReply given an array of // transaction hashes. The implementation may cache the underlying // data if desired. Fully spent transactions will not normally not // be returned in this operation. + // + // NOTE: This function does not return an error directly since it MUST + // return at least one TxListReply instance for each requested + // transaction. Each TxListReply instance then contains an Err field + // which can be used to detect errors. FetchUnSpentTxByShaList(txShaList []*btcwire.ShaHash) []*TxListReply // InsertBlock inserts raw block and transaction data from a block @@ -97,11 +107,11 @@ type Db interface { // RollbackClose discards the recent database changes to the previously // saved data at last Sync and closes the database. - RollbackClose() + RollbackClose() (err error) // Sync verifies that the database is coherent on disk and no // outstanding transactions are in flight. - Sync() + Sync() (err error) } // DriverDB defines a structure for backend drivers to use when they registered diff --git a/interface_test.go b/interface_test.go index 21500a67..31383d56 100644 --- a/interface_test.go +++ b/interface_test.go @@ -85,7 +85,13 @@ func testNewestSha(tc *testContext) bool { // testExistsSha ensures ExistsSha conforms to the interface contract. func testExistsSha(tc *testContext) bool { // The block must exist in the database. - if exists := tc.db.ExistsSha(tc.blockHash); !exists { + exists, err := tc.db.ExistsSha(tc.blockHash) + if err != nil { + tc.t.Errorf("ExistsSha (%s): block #%d (%s) unexpected error: "+ + "%v", tc.dbType, tc.blockHeight, tc.blockHash, err) + return false + } + if !exists { tc.t.Errorf("ExistsSha (%s): block #%d (%s) does not exist", tc.dbType, tc.blockHeight, tc.blockHash) return false @@ -232,7 +238,14 @@ func testExistsTxSha(tc *testContext) bool { for i, tx := range tc.block.Transactions() { // The transaction must exist in the database. txHash := tx.Sha() - if exists := tc.db.ExistsTxSha(txHash); !exists { + exists, err := tc.db.ExistsTxSha(txHash) + if err != nil { + tc.t.Errorf("ExistsTxSha (%s): block #%d (%s) tx #%d "+ + "(%s) unexpected error: %v", tc.dbType, + tc.blockHeight, tc.blockHash, i, txHash, err) + return false + } + if !exists { _, err := tc.db.FetchTxBySha(txHash) if err != nil { tc.t.Errorf("ExistsTxSha (%s): block #%d (%s) "+ diff --git a/ldb/block.go b/ldb/block.go index 25e4c1af..5e5ad56d 100644 --- a/ldb/block.go +++ b/ldb/block.go @@ -11,6 +11,7 @@ import ( "github.com/conformal/btcdb" "github.com/conformal/btcutil" "github.com/conformal/btcwire" + "github.com/conformal/goleveldb/leveldb" ) // FetchBlockBySha - return a btcutil Block @@ -186,30 +187,26 @@ func (db *LevelDb) fetchSha(sha *btcwire.ShaHash) (rbuf []byte, // ExistsSha looks up the given block hash // returns true if it is present in the database. -func (db *LevelDb) ExistsSha(sha *btcwire.ShaHash) (exists bool) { +func (db *LevelDb) ExistsSha(sha *btcwire.ShaHash) (bool, error) { db.dbLock.Lock() defer db.dbLock.Unlock() // not in cache, try database - exists = db.blkExistsSha(sha) - return + return db.blkExistsSha(sha) } // blkExistsSha looks up the given block hash // returns true if it is present in the database. // CALLED WITH LOCK HELD -func (db *LevelDb) blkExistsSha(sha *btcwire.ShaHash) bool { - +func (db *LevelDb) blkExistsSha(sha *btcwire.ShaHash) (bool, error) { _, err := db.getBlkLoc(sha) - - if err != nil { - /* - should this warn if the failure is something besides does not exist ? - log.Warnf("blkExistsSha: fail %v", err) - */ - return false + switch err { + case nil: + return true, nil + case leveldb.ErrNotFound: + return false, nil } - return true + return false, err } // FetchBlockShaByHeight returns a block hash based on its height in the diff --git a/ldb/boundary_test.go b/ldb/boundary_test.go index af49a684..f9ff23c6 100644 --- a/ldb/boundary_test.go +++ b/ldb/boundary_test.go @@ -37,14 +37,20 @@ func TestEmptyDB(t *testing.T) { } // This is a reopen test - db.Close() + if err := db.Close(); err != nil { + t.Errorf("Close: unexpected error: %v", err) + } db, err = btcdb.OpenDB("leveldb", dbname) if err != nil { t.Errorf("Failed to open test database %v", err) return } - defer db.Close() + defer func() { + if err := db.Close(); err != nil { + t.Errorf("Close: unexpected error: %v", err) + } + }() sha, height, err = db.NewestSha() if !sha.IsEqual(&btcwire.ShaHash{}) { diff --git a/ldb/dup_test.go b/ldb/dup_test.go index 8b638dfd..dca2f1fa 100644 --- a/ldb/dup_test.go +++ b/ldb/dup_test.go @@ -29,7 +29,11 @@ func Test_dupTx(t *testing.T) { } defer os.RemoveAll(dbname) defer os.RemoveAll(dbnamever) - defer db.Close() + defer func() { + if err := db.Close(); err != nil { + t.Errorf("Close: unexpected error: %v", err) + } + }() testdatafile := filepath.Join("testdata", "blocks1-256.bz2") blocks, err := loadBlocks(t, testdatafile) @@ -58,7 +62,11 @@ out: origintxsha := &txin.PreviousOutpoint.Hash txneededList = append(txneededList, origintxsha) - if !db.ExistsTxSha(origintxsha) { + exists, err := db.ExistsTxSha(origintxsha) + if err != nil { + t.Errorf("ExistsTxSha: unexpected error %v ", err) + } + if !exists { t.Errorf("referenced tx not found %v ", origintxsha) } diff --git a/ldb/insertremove_test.go b/ldb/insertremove_test.go index 27fdc486..217502ef 100644 --- a/ldb/insertremove_test.go +++ b/ldb/insertremove_test.go @@ -55,7 +55,11 @@ func testUnspentInsert(t *testing.T) { } defer os.RemoveAll(dbname) defer os.RemoveAll(dbnamever) - defer db.Close() + defer func() { + if err := db.Close(); err != nil { + t.Errorf("Close: unexpected error: %v", err) + } + }() blocks := loadblocks(t) endtest: @@ -79,10 +83,13 @@ endtest: txneededList = append(txneededList, origintxsha) txlookupList = append(txlookupList, origintxsha) - if !db.ExistsTxSha(origintxsha) { + exists, err := db.ExistsTxSha(origintxsha) + if err != nil { + t.Errorf("ExistsTxSha: unexpected error %v ", err) + } + if !exists { t.Errorf("referenced tx not found %v ", origintxsha) } - } txshaname, _ := tx.TxSha() txlookupList = append(txlookupList, &txshaname) diff --git a/ldb/leveldb.go b/ldb/leveldb.go index 04ddb474..08490df6 100644 --- a/ldb/leveldb.go +++ b/ldb/leveldb.go @@ -255,26 +255,27 @@ func CreateDB(args ...interface{}) (btcdb.Db, error) { return db, err } -func (db *LevelDb) close() { - db.lDb.Close() +func (db *LevelDb) close() error { + return db.lDb.Close() } // Sync verifies that the database is coherent on disk, // and no outstanding transactions are in flight. -func (db *LevelDb) Sync() { +func (db *LevelDb) Sync() error { db.dbLock.Lock() defer db.dbLock.Unlock() // while specified by the API, does nothing // however does grab lock to verify it does not return until other operations are complete. + return nil } // Close cleanly shuts down database, syncing all data. -func (db *LevelDb) Close() { +func (db *LevelDb) Close() error { db.dbLock.Lock() defer db.dbLock.Unlock() - db.close() + return db.close() } // DropAfterBlockBySha will remove any blocks from the database after @@ -682,9 +683,9 @@ func (db *LevelDb) processBatches() error { return nil } -func (db *LevelDb) RollbackClose() { +func (db *LevelDb) RollbackClose() error { db.dbLock.Lock() defer db.dbLock.Unlock() - db.close() + return db.close() } diff --git a/ldb/operational_test.go b/ldb/operational_test.go index cd8d6395..c0a3ca1e 100644 --- a/ldb/operational_test.go +++ b/ldb/operational_test.go @@ -44,7 +44,11 @@ func testOperationalMode(t *testing.T) { } defer os.RemoveAll(dbname) defer os.RemoveAll(dbnamever) - defer db.Close() + defer func() { + if err := db.Close(); err != nil { + t.Errorf("Close: unexpected error: %v", err) + } + }() testdatafile := filepath.Join("..", "testdata", "blocks1-256.bz2") blocks, err := loadBlocks(t, testdatafile) @@ -67,9 +71,14 @@ out: origintxsha := &txin.PreviousOutpoint.Hash txneededList = append(txneededList, origintxsha) - if !db.ExistsTxSha(origintxsha) { + exists, err := db.ExistsTxSha(origintxsha) + if err != nil { + t.Errorf("ExistsTxSha: unexpected error %v ", err) + } + if !exists { t.Errorf("referenced tx not found %v ", origintxsha) } + _, err = db.FetchTxBySha(origintxsha) if err != nil { t.Errorf("referenced tx not found %v err %v ", origintxsha, err) @@ -178,14 +187,20 @@ func testBackout(t *testing.T) { t.Errorf("Failed to open test database %v", err) return } - defer db.Close() + defer func() { + if err := db.Close(); err != nil { + t.Errorf("Close: unexpected error: %v", err) + } + }() sha, err := blocks[99].Sha() if err != nil { t.Errorf("failed to get block 99 sha err %v", err) return } - _ = db.ExistsSha(sha) + if _, err := db.ExistsSha(sha); err != nil { + t.Errorf("ExistsSha: unexpected error: %v") + } _, err = db.FetchBlockBySha(sha) if err != nil { t.Errorf("failed to load block 99 from db %v", err) @@ -197,7 +212,9 @@ func testBackout(t *testing.T) { t.Errorf("failed to get block 110 sha err %v", err) return } - _ = db.ExistsSha(sha) + if _, err := db.ExistsSha(sha); err != nil { + t.Errorf("ExistsSha: unexpected error: %v") + } _, err = db.FetchBlockBySha(sha) if err != nil { t.Errorf("loaded block 119 from db") @@ -207,7 +224,10 @@ func testBackout(t *testing.T) { block := blocks[119] mblock := block.MsgBlock() txsha, err := mblock.Transactions[0].TxSha() - exists := db.ExistsTxSha(&txsha) + exists, err := db.ExistsTxSha(&txsha) + if err != nil { + t.Errorf("ExistsTxSha: unexpected error %v ", err) + } if !exists { t.Errorf("tx %v not located db\n", txsha) } diff --git a/ldb/tx.go b/ldb/tx.go index 8f5c94c3..8201b1fb 100644 --- a/ldb/tx.go +++ b/ldb/tx.go @@ -147,7 +147,7 @@ func (db *LevelDb) formatTxFullySpent(sTxList []*spentTx) []byte { } // ExistsTxSha returns if the given tx sha exists in the database -func (db *LevelDb) ExistsTxSha(txsha *btcwire.ShaHash) (exists bool) { +func (db *LevelDb) ExistsTxSha(txsha *btcwire.ShaHash) (bool, error) { db.dbLock.Lock() defer db.dbLock.Unlock() @@ -156,15 +156,15 @@ func (db *LevelDb) ExistsTxSha(txsha *btcwire.ShaHash) (exists bool) { // existsTxSha returns if the given tx sha exists in the database.o // Must be called with the db lock held. -func (db *LevelDb) existsTxSha(txSha *btcwire.ShaHash) (exists bool) { +func (db *LevelDb) existsTxSha(txSha *btcwire.ShaHash) (bool, error) { _, _, _, _, err := db.getTxData(txSha) - if err == nil { - return true + switch err { + case nil: + return true, nil + case leveldb.ErrNotFound: + return false, nil } - - // BUG(drahn) If there was an error beside non-existant deal with it. - - return false + return false, err } // FetchTxByShaList returns the most recent tx of the name fully spent or not diff --git a/memdb/memdb.go b/memdb/memdb.go index bd2acb62..bbafd62f 100644 --- a/memdb/memdb.go +++ b/memdb/memdb.go @@ -138,14 +138,19 @@ func (db *MemDb) removeTx(msgTx *btcwire.MsgTx, txHash *btcwire.ShaHash) { // // All data is purged upon close with this implementation since it is a // memory-only database. -func (db *MemDb) Close() { +func (db *MemDb) Close() error { db.Lock() defer db.Unlock() + if db.closed { + return ErrDbClosed + } + db.blocks = nil db.blocksBySha = nil db.txns = nil db.closed = true + return nil } // DropAfterBlockBySha removes any blocks from the database after the given @@ -192,20 +197,19 @@ func (db *MemDb) DropAfterBlockBySha(sha *btcwire.ShaHash) error { // ExistsSha returns whether or not the given block hash is present in the // database. This is part of the btcdb.Db interface implementation. -func (db *MemDb) ExistsSha(sha *btcwire.ShaHash) bool { +func (db *MemDb) ExistsSha(sha *btcwire.ShaHash) (bool, error) { db.Lock() defer db.Unlock() if db.closed { - log.Warnf("ExistsSha called after db close.") - return false + return false, ErrDbClosed } if _, exists := db.blocksBySha[*sha]; exists { - return true + return true, nil } - return false + return false, nil } // FetchBlockBySha returns a btcutil.Block. The implementation may cache the @@ -346,20 +350,19 @@ func (db *MemDb) FetchHeightRange(startHeight, endHeight int64) ([]btcwire.ShaHa // ExistsTxSha returns whether or not the given transaction hash is present in // the database and is not fully spent. This is part of the btcdb.Db interface // implementation. -func (db *MemDb) ExistsTxSha(sha *btcwire.ShaHash) bool { +func (db *MemDb) ExistsTxSha(sha *btcwire.ShaHash) (bool, error) { db.Lock() defer db.Unlock() if db.closed { - log.Warnf("ExistsTxSha called after db close.") - return false + return false, ErrDbClosed } if txns, exists := db.txns[*sha]; exists { - return !isFullySpent(txns[len(txns)-1]) + return !isFullySpent(txns[len(txns)-1]), nil } - return false + return false, nil } // FetchTxBySha returns some data for the given transaction hash. The @@ -702,10 +705,10 @@ func (db *MemDb) NewestSha() (*btcwire.ShaHash, int64, error) { // The database is completely purged on close with this implementation since the // entire database is only in memory. As a result, this function behaves no // differently than Close. -func (db *MemDb) RollbackClose() { +func (db *MemDb) RollbackClose() error { // Rollback doesn't apply to a memory database, so just call Close. // Close handles the mutex locks. - db.Close() + return db.Close() } // Sync verifies that the database is coherent on disk and no outstanding @@ -714,18 +717,18 @@ func (db *MemDb) RollbackClose() { // // This implementation does not write any data to disk, so this function only // grabs a lock to ensure it doesn't return until other operations are complete. -func (db *MemDb) Sync() { +func (db *MemDb) Sync() error { db.Lock() defer db.Unlock() if db.closed { - log.Warnf("Sync called after db close.") + return ErrDbClosed } // There is nothing extra to do to sync the memory database. However, // the lock is still grabbed to ensure the function does not return // until other operations are complete. - return + return nil } // newMemDb returns a new memory-only database ready for block inserts. diff --git a/memdb/memdb_test.go b/memdb/memdb_test.go index 8e33de47..ddbe3717 100644 --- a/memdb/memdb_test.go +++ b/memdb/memdb_test.go @@ -29,15 +29,17 @@ func TestClosed(t *testing.T) { if err != nil { t.Errorf("InsertBlock: %v", err) } - db.Close() + if err := db.Close(); err != nil { + t.Errorf("Close: unexpected error %v", err) + } genesisHash := btcnet.MainNetParams.GenesisHash if err := db.DropAfterBlockBySha(genesisHash); err != memdb.ErrDbClosed { t.Errorf("DropAfterBlockBySha: unexpected error %v", err) } - if exists := db.ExistsSha(genesisHash); exists != false { - t.Errorf("ExistsSha: genesis hash exists after close") + if _, err := db.ExistsSha(genesisHash); err != memdb.ErrDbClosed { + t.Errorf("ExistsSha: Unexpected error: %v", err) } if _, err := db.FetchBlockBySha(genesisHash); err != memdb.ErrDbClosed { @@ -57,9 +59,8 @@ func TestClosed(t *testing.T) { if err != nil { t.Errorf("TxSha: unexpected error %v", err) } - if exists := db.ExistsTxSha(&coinbaseHash); exists != false { - t.Errorf("ExistsTxSha: hash %v exists when it shouldn't", - &coinbaseHash) + if _, err := db.ExistsTxSha(&coinbaseHash); err != memdb.ErrDbClosed { + t.Errorf("ExistsTxSha: unexpected error %v", err) } if _, err := db.FetchTxBySha(genesisHash); err != memdb.ErrDbClosed { @@ -103,9 +104,15 @@ func TestClosed(t *testing.T) { t.Errorf("NewestSha: unexpected error %v", err) } - // The following calls don't return errors from the interface to be able - // to detect a closed database, so just call them to ensure there are no - // panics. - db.Sync() - db.RollbackClose() + if err := db.Sync(); err != memdb.ErrDbClosed { + t.Errorf("Sync: unexpected error %v", err) + } + + if err := db.RollbackClose(); err != memdb.ErrDbClosed { + t.Errorf("RollbackClose: unexpected error %v", err) + } + + if err := db.Close(); err != memdb.ErrDbClosed { + t.Errorf("Close: unexpected error %v", err) + } }