Merge #7927: Minor changes to dbwrapper to simplify support for other databases

laanwj · laanwj · commit f9c2ac72327e · 2016-04-25T12:46:23.000+02:00
869cf12 dbwrapper: Move `HandleError` to `dbwrapper_private` (Wladimir J. van der Laan) b69836d dbwrapper: Pass parent CDBWrapper into CDBBatch and CDBIterator (Wladimir J. van der Laan) 878bf48 dbwrapper: Remove CDBWrapper::GetObfuscateKeyHex (Wladimir J. van der Laan) 74f7b12 dbwrapper: Remove throw keywords in function signatures (Wladimir J. van der Laan)
diff --git a/src/dbwrapper.cpp b/src/dbwrapper.cpp
@@ -15,20 +15,6 @@
 #include <memenv.h>
 #include <stdint.h>
 
-void HandleError(const leveldb::Status& status) throw(dbwrapper_error)
-{
-    if (status.ok())
-        return;
-    LogPrintf("%s\n", status.ToString());
-    if (status.IsCorruption())
-        throw dbwrapper_error("Database corrupted");
-    if (status.IsIOError())
-        throw dbwrapper_error("Database I/O error");
-    if (status.IsNotFound())
-        throw dbwrapper_error("Database entry missing");
-    throw dbwrapper_error("Unknown database error");
-}
-
 static leveldb::Options GetOptions(size_t nCacheSize)
 {
     leveldb::Options options;
@@ -61,13 +47,13 @@ CDBWrapper::CDBWrapper(const boost::filesystem::path& path, size_t nCacheSize, b
         if (fWipe) {
             LogPrintf("Wiping LevelDB in %s\n", path.string());
             leveldb::Status result = leveldb::DestroyDB(path.string(), options);
-            HandleError(result);
+            dbwrapper_private::HandleError(result);
         }
         TryCreateDirectory(path);
         LogPrintf("Opening LevelDB in %s\n", path.string());
     }
     leveldb::Status status = leveldb::DB::Open(options, path.string(), &pdb);
-    HandleError(status);
+    dbwrapper_private::HandleError(status);
     LogPrintf("Opened LevelDB successfully\n");
 
     // The base-case obfuscation key, which is a noop.
@@ -84,10 +70,10 @@ CDBWrapper::CDBWrapper(const boost::filesystem::path& path, size_t nCacheSize, b
         Write(OBFUSCATE_KEY_KEY, new_key);
         obfuscate_key = new_key;
 
-        LogPrintf("Wrote new obfuscate key for %s: %s\n", path.string(), GetObfuscateKeyHex());
+        LogPrintf("Wrote new obfuscate key for %s: %s\n", path.string(), HexStr(obfuscate_key));
     }
 
-    LogPrintf("Using obfuscation key for %s: %s\n", path.string(), GetObfuscateKeyHex());
+    LogPrintf("Using obfuscation key for %s: %s\n", path.string(), HexStr(obfuscate_key));
 }
 
 CDBWrapper::~CDBWrapper()
@@ -102,10 +88,10 @@ CDBWrapper::~CDBWrapper()
     options.env = NULL;
 }
 
-bool CDBWrapper::WriteBatch(CDBBatch& batch, bool fSync) throw(dbwrapper_error)
+bool CDBWrapper::WriteBatch(CDBBatch& batch, bool fSync)
 {
     leveldb::Status status = pdb->Write(fSync ? syncoptions : writeoptions, &batch.batch);
-    HandleError(status);
+    dbwrapper_private::HandleError(status);
     return true;
 }
 
@@ -136,17 +122,30 @@ bool CDBWrapper::IsEmpty()
     return !(it->Valid());
 }
 
-const std::vector<unsigned char>& CDBWrapper::GetObfuscateKey() const
+CDBIterator::~CDBIterator() { delete piter; }
+bool CDBIterator::Valid() { return piter->Valid(); }
+void CDBIterator::SeekToFirst() { piter->SeekToFirst(); }
+void CDBIterator::Next() { piter->Next(); }
+
+namespace dbwrapper_private {
+
+void HandleError(const leveldb::Status& status)
 {
-    return obfuscate_key;
+    if (status.ok())
+        return;
+    LogPrintf("%s\n", status.ToString());
+    if (status.IsCorruption())
+        throw dbwrapper_error("Database corrupted");
+    if (status.IsIOError())
+        throw dbwrapper_error("Database I/O error");
+    if (status.IsNotFound())
+        throw dbwrapper_error("Database entry missing");
+    throw dbwrapper_error("Unknown database error");
 }
 
-std::string CDBWrapper::GetObfuscateKeyHex() const
+const std::vector<unsigned char>& GetObfuscateKey(const CDBWrapper &w)
 {
-    return HexStr(obfuscate_key);
+    return w.obfuscate_key;
 }
 
-CDBIterator::~CDBIterator() { delete piter; }
-bool CDBIterator::Valid() { return piter->Valid(); }
-void CDBIterator::SeekToFirst() { piter->SeekToFirst(); }
-void CDBIterator::Next() { piter->Next(); }
+};
diff --git a/src/dbwrapper.h b/src/dbwrapper.h
@@ -23,22 +23,38 @@ class dbwrapper_error : public std::runtime_error
     dbwrapper_error(const std::string& msg) : std::runtime_error(msg) {}
 };
 
-void HandleError(const leveldb::Status& status) throw(dbwrapper_error);
+class CDBWrapper;
+
+/** These should be considered an implementation detail of the specific database.
+ */
+namespace dbwrapper_private {
+
+/** Handle database error by throwing dbwrapper_error exception.
+ */
+void HandleError(const leveldb::Status& status);
+
+/** Work around circular dependency, as well as for testing in dbwrapper_tests.
+ * Database obfuscation should be considered an implementation detail of the
+ * specific database.
+ */
+const std::vector<unsigned char>& GetObfuscateKey(const CDBWrapper &w);
+
+};
 
 /** Batch of changes queued to be written to a CDBWrapper */
 class CDBBatch
 {
     friend class CDBWrapper;
 
 private:
+    const CDBWrapper &parent;
     leveldb::WriteBatch batch;
-    const std::vector<unsigned char> *obfuscate_key;
 
 public:
     /**
-     * @param[in] obfuscate_key    If passed, XOR data with this key.
+     * @param[in] parent    CDBWrapper that this batch is to be submitted to
      */
-    CDBBatch(const std::vector<unsigned char> *obfuscate_key) : obfuscate_key(obfuscate_key) { };
+    CDBBatch(const CDBWrapper &parent) : parent(parent) { };
 
     template <typename K, typename V>
     void Write(const K& key, const V& value)
@@ -51,7 +67,7 @@ class CDBBatch
         CDataStream ssValue(SER_DISK, CLIENT_VERSION);
         ssValue.reserve(ssValue.GetSerializeSize(value));
         ssValue << value;
-        ssValue.Xor(*obfuscate_key);
+        ssValue.Xor(dbwrapper_private::GetObfuscateKey(parent));
         leveldb::Slice slValue(&ssValue[0], ssValue.size());
 
         batch.Put(slKey, slValue);
@@ -72,17 +88,17 @@ class CDBBatch
 class CDBIterator
 {
 private:
+    const CDBWrapper &parent;
     leveldb::Iterator *piter;
-    const std::vector<unsigned char> *obfuscate_key;
 
 public:
 
     /**
+     * @param[in] parent           Parent CDBWrapper instance.
      * @param[in] piterIn          The original leveldb iterator.
-     * @param[in] obfuscate_key    If passed, XOR data with this key.
      */
-    CDBIterator(leveldb::Iterator *piterIn, const std::vector<unsigned char>* obfuscate_key) :
-        piter(piterIn), obfuscate_key(obfuscate_key) { };
+    CDBIterator(const CDBWrapper &parent, leveldb::Iterator *piterIn) :
+        parent(parent), piter(piterIn) { };
     ~CDBIterator();
 
     bool Valid();
@@ -118,7 +134,7 @@ class CDBIterator
         leveldb::Slice slValue = piter->value();
         try {
             CDataStream ssValue(slValue.data(), slValue.data() + slValue.size(), SER_DISK, CLIENT_VERSION);
-            ssValue.Xor(*obfuscate_key);
+            ssValue.Xor(dbwrapper_private::GetObfuscateKey(parent));
             ssValue >> value;
         } catch (const std::exception&) {
             return false;
@@ -134,6 +150,7 @@ class CDBIterator
 
 class CDBWrapper
 {
+    friend const std::vector<unsigned char>& dbwrapper_private::GetObfuscateKey(const CDBWrapper &w);
 private:
     //! custom environment this database is using (may be NULL in case of default environment)
     leveldb::Env* penv;
@@ -180,7 +197,7 @@ class CDBWrapper
     ~CDBWrapper();
 
     template <typename K, typename V>
-    bool Read(const K& key, V& value) const throw(dbwrapper_error)
+    bool Read(const K& key, V& value) const
     {
         CDataStream ssKey(SER_DISK, CLIENT_VERSION);
         ssKey.reserve(ssKey.GetSerializeSize(key));
@@ -193,7 +210,7 @@ class CDBWrapper
             if (status.IsNotFound())
                 return false;
             LogPrintf("LevelDB read failure: %s\n", status.ToString());
-            HandleError(status);
+            dbwrapper_private::HandleError(status);
         }
         try {
             CDataStream ssValue(strValue.data(), strValue.data() + strValue.size(), SER_DISK, CLIENT_VERSION);
@@ -206,15 +223,15 @@ class CDBWrapper
     }
 
     template <typename K, typename V>
-    bool Write(const K& key, const V& value, bool fSync = false) throw(dbwrapper_error)
+    bool Write(const K& key, const V& value, bool fSync = false)
     {
-        CDBBatch batch(&obfuscate_key);
+        CDBBatch batch(*this);
         batch.Write(key, value);
         return WriteBatch(batch, fSync);
     }
 
     template <typename K>
-    bool Exists(const K& key) const throw(dbwrapper_error)
+    bool Exists(const K& key) const
     {
         CDataStream ssKey(SER_DISK, CLIENT_VERSION);
         ssKey.reserve(ssKey.GetSerializeSize(key));
@@ -227,53 +244,42 @@ class CDBWrapper
             if (status.IsNotFound())
                 return false;
             LogPrintf("LevelDB read failure: %s\n", status.ToString());
-            HandleError(status);
+            dbwrapper_private::HandleError(status);
         }
         return true;
     }
 
     template <typename K>
-    bool Erase(const K& key, bool fSync = false) throw(dbwrapper_error)
+    bool Erase(const K& key, bool fSync = false)
     {
-        CDBBatch batch(&obfuscate_key);
+        CDBBatch batch(*this);
         batch.Erase(key);
         return WriteBatch(batch, fSync);
     }
 
-    bool WriteBatch(CDBBatch& batch, bool fSync = false) throw(dbwrapper_error);
+    bool WriteBatch(CDBBatch& batch, bool fSync = false);
 
     // not available for LevelDB; provide for compatibility with BDB
     bool Flush()
     {
         return true;
     }
 
-    bool Sync() throw(dbwrapper_error)
+    bool Sync()
     {
-        CDBBatch batch(&obfuscate_key);
+        CDBBatch batch(*this);
         return WriteBatch(batch, true);
     }
 
     CDBIterator *NewIterator()
     {
-        return new CDBIterator(pdb->NewIterator(iteroptions), &obfuscate_key);
+        return new CDBIterator(*this, pdb->NewIterator(iteroptions));
     }
 
     /**
      * Return true if the database managed by this class contains no entries.
      */
     bool IsEmpty();
-
-    /**
-     * Accessor for obfuscate_key.
-     */
-    const std::vector<unsigned char>& GetObfuscateKey() const;
-
-    /**
-     * Return the obfuscate_key as a hex-formatted string.
-     */
-    std::string GetObfuscateKeyHex() const;
-
 };
 
 #endif // BITCOIN_DBWRAPPER_H
diff --git a/src/test/dbwrapper_tests.cpp b/src/test/dbwrapper_tests.cpp
@@ -39,7 +39,7 @@ BOOST_AUTO_TEST_CASE(dbwrapper)
         uint256 res;
 
         // Ensure that we're doing real obfuscation when obfuscate=true
-        BOOST_CHECK(obfuscate != is_null_key(dbw.GetObfuscateKey()));
+        BOOST_CHECK(obfuscate != is_null_key(dbwrapper_private::GetObfuscateKey(dbw)));
 
         BOOST_CHECK(dbw.Write(key, in));
         BOOST_CHECK(dbw.Read(key, res));
@@ -64,7 +64,7 @@ BOOST_AUTO_TEST_CASE(dbwrapper_batch)
         uint256 in3 = GetRandHash();
 
         uint256 res;
-        CDBBatch batch(&dbw.GetObfuscateKey());
+        CDBBatch batch(dbw);
 
         batch.Write(key, in);
         batch.Write(key2, in2);
@@ -156,7 +156,7 @@ BOOST_AUTO_TEST_CASE(existing_data_no_obfuscate)
     BOOST_CHECK_EQUAL(res2.ToString(), in.ToString());
 
     BOOST_CHECK(!odbw.IsEmpty()); // There should be existing data
-    BOOST_CHECK(is_null_key(odbw.GetObfuscateKey())); // The key should be an empty string
+    BOOST_CHECK(is_null_key(dbwrapper_private::GetObfuscateKey(odbw))); // The key should be an empty string
 
     uint256 in2 = GetRandHash();
     uint256 res3;
@@ -193,7 +193,7 @@ BOOST_AUTO_TEST_CASE(existing_data_reindex)
     // Check that the key/val we wrote with unobfuscated wrapper doesn't exist
     uint256 res2;
     BOOST_CHECK(!odbw.Read(key, res2));
-    BOOST_CHECK(!is_null_key(odbw.GetObfuscateKey()));
+    BOOST_CHECK(!is_null_key(dbwrapper_private::GetObfuscateKey(odbw)));
 
     uint256 in2 = GetRandHash();
     uint256 res3;
diff --git a/src/txdb.cpp b/src/txdb.cpp
@@ -49,7 +49,7 @@ uint256 CCoinsViewDB::GetBestBlock() const {
 }
 
 bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) {
-    CDBBatch batch(&db.GetObfuscateKey());
+    CDBBatch batch(db);
     size_t count = 0;
     size_t changed = 0;
     for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end();) {
@@ -139,7 +139,7 @@ void CCoinsViewDBCursor::Next()
 }
 
 bool CBlockTreeDB::WriteBatchSync(const std::vector<std::pair<int, const CBlockFileInfo*> >& fileInfo, int nLastFile, const std::vector<const CBlockIndex*>& blockinfo) {
-    CDBBatch batch(&GetObfuscateKey());
+    CDBBatch batch(*this);
     for (std::vector<std::pair<int, const CBlockFileInfo*> >::const_iterator it=fileInfo.begin(); it != fileInfo.end(); it++) {
         batch.Write(make_pair(DB_BLOCK_FILES, it->first), *it->second);
     }
@@ -155,7 +155,7 @@ bool CBlockTreeDB::ReadTxIndex(const uint256 &txid, CDiskTxPos &pos) {
 }
 
 bool CBlockTreeDB::WriteTxIndex(const std::vector<std::pair<uint256, CDiskTxPos> >&vect) {
-    CDBBatch batch(&GetObfuscateKey());
+    CDBBatch batch(*this);
     for (std::vector<std::pair<uint256,CDiskTxPos> >::const_iterator it=vect.begin(); it!=vect.end(); it++)
         batch.Write(make_pair(DB_TXINDEX, it->first), it->second);
     return WriteBatch(batch);

Original file line number	Diff line number	Diff line change
`@@ -49,7 +49,7 @@ uint256 CCoinsViewDB::GetBestBlock() const {`
`49`	`49`	`}`
`50`	`50`
`51`	`51`	`bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) {`
`52`		`- CDBBatch batch(&db.GetObfuscateKey());`
	`52`	`+ CDBBatch batch(db);`
`53`	`53`	`size_t count = 0;`
`54`	`54`	`size_t changed = 0;`
`55`	`55`	`for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end();) {`
`@@ -139,7 +139,7 @@ void CCoinsViewDBCursor::Next()`
`139`	`139`	`}`
`140`	`140`
`141`	`141`	`bool CBlockTreeDB::WriteBatchSync(const std::vector<std::pair<int, const CBlockFileInfo> >& fileInfo, int nLastFile, const std::vector<const CBlockIndex>& blockinfo) {`
`142`		`- CDBBatch batch(&GetObfuscateKey());`
	`142`	`+ CDBBatch batch(*this);`
`143`	`143`	`for (std::vector<std::pair<int, const CBlockFileInfo*> >::const_iterator it=fileInfo.begin(); it != fileInfo.end(); it++) {`
`144`	`144`	`batch.Write(make_pair(DB_BLOCK_FILES, it->first), *it->second);`
`145`	`145`	`}`
`@@ -155,7 +155,7 @@ bool CBlockTreeDB::ReadTxIndex(const uint256 &txid, CDiskTxPos &pos) {`
`155`	`155`	`}`
`156`	`156`
`157`	`157`	`bool CBlockTreeDB::WriteTxIndex(const std::vector<std::pair<uint256, CDiskTxPos> >&vect) {`
`158`		`- CDBBatch batch(&GetObfuscateKey());`
	`158`	`+ CDBBatch batch(*this);`
`159`	`159`	`for (std::vector<std::pair<uint256,CDiskTxPos> >::const_iterator it=vect.begin(); it!=vect.end(); it++)`
`160`	`160`	`batch.Write(make_pair(DB_TXINDEX, it->first), it->second);`
`161`	`161`	`return WriteBatch(batch);`