Wallet database handling abstractions/simplifications

laanwj commented at 2:47 pm on March 8, 2017: member

My immediate goal with this is to make it easier to use a different key/value store instead of BerkeleyDB, for a sandboxing project. However, it is an improvement also for BerkeleyDB.

Steps:

Abstract database handle from explicit strFilename into CWalletDBWrapper. For many databases, a usable handle involves more than just a filename. Also make the field private.
Get rid of fFileBacked - it is only used by the tests, and used inconsistly - for example CWallet::AddToWallet doesn’t check it at all, and that’s not quite the only place (see AbandonWallet). Fixing would be a lot of work and lead to disjointed code.

But it is not necessary. Instead move the concern to ignoring operations with a dummy database to CDB. There were very few changes necessary for this.
Reduce references to global bitdb. CWalletDBWrapper carries this environment information around so there is less need to refer to global scope. Besides being cleaner, this could allow use of multiple database environments at some point.
CWalletDB now contains a CDB instead of inheriting from it. This makes it easier to replace the internal transaction with a different database, without leaking through internals.

Future:

The eventual goal of “CWalletDBWrapper” is like “CDBWrapper”, to move further database functionality there from “CDB” (which is really a “CDBBatch”, a database transaction object). Not renaming it nor CWalletDB here to not interfere unduly much with other open wallet pulls.
Two maps inside bitdb that refer to wallet files by name could be removed, and converted to fields on CWalletDBWrapper:

std::map<std::string, int> mapFileUseCount; std::map<std::string, Db*> mapDb;

laanwj added the label Wallet on Mar 8, 2017

jonasschnelli commented at 8:02 am on March 9, 2017: contributor

Great! Concept ACK. Needs rebase.

laanwj force-pushed on Mar 9, 2017

laanwj commented at 8:18 am on March 9, 2017: member

Yep needed trivial rebase for #9643.

in src/wallet/walletdb.h: in 4ab3a72687 outdated

121+ */
122+class CWalletDB
123 {
124 public:
125-    CWalletDB(const std::string& strFilename, const char* pszMode = "r+", bool _fFlushOnClose = true) : CDB(strFilename, pszMode, _fFlushOnClose)
126+    /** Takes a std::unique_ptr reference to avoid having to use

laanwj commented at 9:35 am on March 9, 2017:

This unique_ptr here bothers me. The interface shouldn’t care what kind of pointer is used. I think I’m going to change this to take a CWalletDBWrapper&, then replace the call sites with CWalletDB(*dbw). That’s better than CWalletDB(dbw.get()) at least. Edit: done

laanwj force-pushed on Mar 9, 2017

in src/wallet/db.cpp: in 757f0a7261 outdated

365     int ret;
366     fReadOnly = (!strchr(pszMode, '+') && !strchr(pszMode, 'w'));
367     fFlushOnClose = fFlushOnCloseIn;
368-    if (strFilename.empty())
369-        return;
370+    std::string strFilename = dbw.strWalletFile;

benma commented at 3:32 pm on March 9, 2017:

Better to use const std::string& strFileName, no copying and it’s clear it won’t be modified.

laanwj commented at 4:09 pm on March 9, 2017:

Agreed.

in src/wallet/db.cpp: in 757f0a7261 outdated

470@@ -472,8 +471,11 @@ bool CDBEnv::RemoveDb(const std::string& strFile)
471     return (rc == 0);
472 }
473 
474-bool CDB::Rewrite(const std::string& strFile, const char* pszSkip)
475+bool CDB::Rewrite(CWalletDBWrapper &dbw, const char* pszSkip)
476 {
477+    if (!dbw.env)
478+        return true;
479+    std::string strFile = dbw.strWalletFile;

benma commented at 3:32 pm on March 9, 2017:

same

in src/wallet/db.cpp: in 757f0a7261 outdated

603 {
604     bool ret = false;
605+    if (!dbw) {
606+        return true;
607+    }
608+    std::string strFile = dbw->strWalletFile;

benma commented at 3:32 pm on March 9, 2017:

same

in src/wallet/db.cpp: in 757f0a7261 outdated

597@@ -596,9 +598,13 @@ void CDBEnv::Flush(bool fShutdown)
598     }
599 }
600 
601-bool CDB::PeriodicFlush(std::string strFile)
602+bool CDB::PeriodicFlush(CWalletDBWrapper *dbw)
603 {
604     bool ret = false;
605+    if (!dbw) {

benma commented at 3:34 pm on March 9, 2017:

Better to move the check to the top, above bool ret.

in src/wallet/db.cpp: in 757f0a7261 outdated

645+    return CDB::Rewrite(*this, pszSkip);
646+}
647+
648+bool CWalletDBWrapper::Backup(const std::string& strDest)
649+{
650+    if (!env)

benma commented at 3:35 pm on March 9, 2017:

Inconsistent style with using/skipping braces (above, you used braces, here, you don’t). I’d add them.

laanwj commented at 4:10 pm on March 9, 2017:

Yes the brace should be after

in src/wallet/db.h: in 757f0a7261 outdated

109+    /** Get a name for this database, for debugging etc.
110+     */
111+    std::string GetName() { return strWalletFile; }
112+
113+    CDBEnv *env;
114+    std::string strWalletFile;

benma commented at 3:37 pm on March 9, 2017:

const std::string

in src/wallet/db.h: in 757f0a7261 outdated

106+     */
107+    bool Backup(const std::string& strDest);
108+
109+    /** Get a name for this database, for debugging etc.
110+     */
111+    std::string GetName() { return strWalletFile; }

benma commented at 3:38 pm on March 9, 2017:

std::string GetName() const

You have this function, and the public strWalletFile, which you access directly in other places. Maybe strWalletFile should be made private, or alternatively, this function removed.

laanwj commented at 4:04 pm on March 9, 2017:

They are subtly different, only happen to be the same for BerkeleyDB:

GetName is an an abstract name to refer to the database object for logging or troubleshooting. It should exist for any version of CDBWrapper.
strWalletFile is an implementation detail that only exists for BerkeleyDB. Other databases may have completely different connection descriptions, which do not involve simply a string. So this is private to the low-level implementation (CDB, CWalletDBWrapper), the higher-level stuff (such as CWallet) should not refer to it.

laanwj commented at 4:07 pm on March 9, 2017:

Could make this clearer using “friend” I guess.

in src/wallet/wallet.h: in 757f0a7261 outdated

674+        return dbw.get();
675+    }
676+
677+    /** Get a name for this wallet for logging/debugging purposes.
678+     */
679+    std::string GetName()

benma commented at 3:40 pm on March 9, 2017:

GetName() const

laanwj commented at 4:11 pm on March 9, 2017:

Yes good catch.

in src/wallet/wallet.h: in 757f0a7261 outdated

678+     */
679+    std::string GetName()
680+    {
681+        if (dbw) {
682+            return dbw->GetName();
683+        } else {

benma commented at 3:41 pm on March 9, 2017:

Maybe it is a matter of personal preference, but I always find it odd to put an else when the if returns.

laanwj commented at 4:06 pm on March 9, 2017:

I prefer to keep it this way. We have no guideline on this in doc/developer-notes.md, and I personally find it easier to follow the control flow this way.

benma commented at 3:44 pm on March 9, 2017: none

Some comments on the first commit.

Since this is my first review, please let me know if it is overboard/too pedantic.

laanwj force-pushed on Mar 9, 2017

laanwj commented at 4:46 pm on March 9, 2017: member

Fixed @benma’s nits.

laanwj force-pushed on Mar 9, 2017

in src/wallet/db.h: in b2db6c3a08 outdated

112+    std::string GetName() const { return strFile; }
113+
114+private:
115+    /** BerkeleyDB specific */
116+    CDBEnv *env;
117+    std::string strFile;

benma commented at 5:20 pm on March 9, 2017:

const std::string strFile; ?

benma commented at 6:06 pm on March 9, 2017: none

For my understanding: the refactor to support a different db backend than BerkeleyDB is not finished with this PR, right? It is a bit hard to see through CDBEnv, CDB and CWalletDBWrapper, but CDB looks half-finished: it still refers to the walletFile string in most functions instead of going through CWalletDBWrapper.

Also, berkeley-specific fields are in CWalletDBWrapper. If you want to use a different backend that doesn’t rely on a filename, where would that fit in? Would that end up in two classes inheriting from CWalletDBWrapper, and moving the berkeley-specific stuff out of it?

The eventual goal of “CWalletDBWrapper” is like “CDBWrapper”, to move further database functionality there from “CDB” (which is really a “CDBBatch”, a database transaction object). Not renaming it nor CWalletDB here to not interfere unduly much with other open wallet pull

In the end, is there even a need for the class CDB anymore?

laanwj commented at 8:15 pm on March 9, 2017: member

For my understanding: the refactor to support a different db backend than BerkeleyDB is not finished with this PR, right? It is a bit hard to see through CDBEnv, CDB and CWalletDBWrapper, but CDB looks half-finished: it still refers to the walletFile string in most functions instead of going through CWalletDBWrapper.

To be clear: CDB and CWalletDBWrapper and CDBEnv are database specific. You need to replace those if you want to change the backend (though you should keep the interface of the first two the same). In many cases you can do without CDBEnv (for LevelDB I could just delete it). So db.h/cpp is database specific, walletdb.h/cpp and wallet.h/cpp are not.

On top of this I have a patch that switches the backend to LevelDB, mostly changing only db.cpp/db.h (completely) and CWalletDB constructor. I need this for testing, to be clear I’m not going to PR that for merging.

It’s not quite as easy as switching the dbwrapper.h backend yet, but much easier than before.

In the end, is there even a need for the class CDB anymore?

Yes, it is a database transaction / batch. It’s simlar to CDBBatch.

laanwj commented at 6:46 am on March 10, 2017: member

@benma FYI not for review but if you’re interested https://github.com/laanwj/bitcoin/tree/20170310_experiment_leveldb_wallet is a leveldb wallet on top of this (edit: rebased, is on top of this version now). It passes the unit and QA tests apart from backupwallet which isn’t implemented.

in src/wallet/db.h: in e3e51fdd3a outdated

164@@ -118,7 +165,7 @@ class CDB
165     CDB(const CDB&);

TheBlueMatt commented at 8:16 pm on March 10, 2017:

Can you go ahead and =delete these, since we have C++11?

laanwj commented at 7:04 am on March 12, 2017:

Good idea

TheBlueMatt commented at 8:22 pm on March 10, 2017: member

Dear god this stuffs a mess, good thing you’re cleaing it up :P.

utACK e3e51fdd3a9491ab4a07baed0a0f52b47ef70140

sipa commented at 6:35 am on March 12, 2017: member

Concept ACK

jonasschnelli commented at 1:39 pm on March 27, 2017: contributor

Tested ACK e3e51fdd3a9491ab4a07baed0a0f52b47ef70140 (https://bitcoin.jonasschnelli.ch/build/PR/9951)

Though needs a “trivial” rebase.

in src/wallet/db.h:97 in 58ad2ead8b outdated

93@@ -94,6 +94,12 @@ class CWalletDBWrapper
94 {
95     friend class CDB;
96 public:
97+    /** Create dummy DB handle */
98+    CWalletDBWrapper(): env(nullptr)

JeremyRubin commented at 4:51 pm on April 5, 2017:

I think there’s a small benefit to not making a CWalletDBWrapper the default constructor. Maybe delete the default, and have struct MockTag{}; CWalletDBWrapper(MockTag); to make a dummy explicitly constructed.

in src/wallet/db.cpp:367 in b2db6c3a08 outdated

365     int ret;
366     fReadOnly = (!strchr(pszMode, '+') && !strchr(pszMode, 'w'));
367     fFlushOnClose = fFlushOnCloseIn;
368-    if (strFilename.empty())
369-        return;
370+    const std::string& strFilename = dbw.strFile;

JeremyRubin commented at 6:53 pm on April 5, 2017:

Previously, an empty strFilename would return immediately. Also, given that we initialize strFile to copy strFileName, we may as well just toss this into the constructor’s initialize list. If it’s empty, it’s free. If it’s not empty, we were going to copy it anyways.

in src/wallet/db.cpp:362 in b2db6c3a08 outdated

358@@ -359,13 +359,12 @@ void CDBEnv::CheckpointLSN(const std::string& strFile)
359 }
360 
361 
362-CDB::CDB(const std::string& strFilename, const char* pszMode, bool fFlushOnCloseIn) : pdb(NULL), activeTxn(NULL)
363+CDB::CDB(CWalletDBWrapper& dbw, const char* pszMode, bool fFlushOnCloseIn) : pdb(NULL), activeTxn(NULL)

JeremyRubin commented at 7:10 pm on April 5, 2017:

I’m a little confused on the API here.

I feel like the unwrapped version of CDB shouldn’t be compatible with the wrapped version of CWalletDB?

laanwj commented at 3:06 pm on April 20, 2017:

CWalletDBWrapper is not a wrapper around CWalletDB. We kind of have a naming problem with the wallet:

CDBEnv is an environment in which the database exists (has no analog in dbwrapper.h)
CWalletDBWrapper represents a wallet database (similar to CDBWrapper in dbwrapper.h)
CDB is a low-level database transaction (similar to CDBBatch in dbwrapper.h)
CWalletDB is a modifier object for the wallet, and encapsulates a database transaction as well as methods to act on the database (no analog in dbwrapper.h)

The latter two are named badly, should be renamed at some point but didn’t want to do it here.

laanwj commented at 2:05 pm on April 21, 2017:

I added a commit that adds this as a comment, as it will be something that confuses everyone reading this code.

in src/wallet/db.h:124 in 58ad2ead8b outdated

116@@ -111,6 +117,12 @@ class CWalletDBWrapper
117      */
118     std::string GetName() const { return strFile; }
119 
120+    /** Return whether this database handle is a dummy for testing.
121+     * Only to be used at a low level, application should ideally not care
122+     * about this.
123+     */
124+    bool IsDummy() { return env == nullptr; }

JeremyRubin commented at 7:14 pm on April 5, 2017:

If it’s only to be used by low level stuff, maybe friend the function specifically to those classes…

laanwj commented at 3:47 pm on April 20, 2017:

Yes, good point. CDB is already a friend and should be the only user of this, moving it to private.

in src/wallet/walletdb.cpp:36 in e3e51fdd3a outdated

32@@ -33,46 +33,46 @@ static std::atomic<unsigned int> nWalletDBUpdateCounter;
33 bool CWalletDB::WriteName(const std::string& strAddress, const std::string& strName)
34 {
35     nWalletDBUpdateCounter++;
36-    return Write(make_pair(std::string("name"), strAddress), strName);
37+    return batch.Write(make_pair(std::string("name"), strAddress), strName);

JeremyRubin commented at 7:20 pm on April 5, 2017:

missing std::

laanwj commented at 3:52 pm on April 20, 2017:

I don’t understand why this doesn’t cause a compile error. We’ve removed all the “using namespace std”.

JeremyRubin approved

JeremyRubin commented at 7:26 pm on April 5, 2017: contributor

Couple nits, overall utack.

jonasschnelli commented at 10:23 am on April 19, 2017: contributor

Needs rebase (Imo this is ready for merge).

laanwj commented at 10:44 am on April 19, 2017: member

Yes, will rebase this soon(TM) and fix the nits.

wallet: Introduce database handle wrapper

Abstract database handle from explicit strFilename into
CWalletDBWrapper.

Also move CWallet::Backup to db.cpp - as it deals with representation
details this is a database specific operation.

71afe3c099

wallet: Get rid of fFileBacked

Instead, CWalletDB() with a dummy handle will just give you a no-op
database in which writes always succeeds and reads always fail. CDB
already had functionality for this, so just use that.

071c95570b

wallet: Reduce references to global bitdb environment be9e1a968d

wallet: CWalletDB CDB composition not inheritance

CWalletDB now contains a CDB instead of inheriting from it.

This makes it easier to replace the internal transaction with a different
database, without leaking through internals.

33232810dc

wallet: Make IsDummy private in CWalletDBWrapper

This is only for use in the low-level functions, and CDB is already
a friend class.

69d2e9ba67

laanwj force-pushed on Apr 20, 2017

wallet: Add comment describing the various classes in walletdb.h 911a4808fb

laanwj merged this on Apr 24, 2017

laanwj closed this on Apr 24, 2017

laanwj referenced this in commit fa1ac2881f on Apr 24, 2017

PastaPastaPasta referenced this in commit 90cfebc065 on May 21, 2019

PastaPastaPasta referenced this in commit 1f55f65812 on May 21, 2019

PastaPastaPasta referenced this in commit e0872bec91 on May 22, 2019

PastaPastaPasta referenced this in commit 25f1ba1258 on May 22, 2019

PastaPastaPasta referenced this in commit 7f62d8817f on May 22, 2019

PastaPastaPasta referenced this in commit cf79ef845c on May 22, 2019

PastaPastaPasta referenced this in commit 8b9dc89b93 on May 22, 2019

PastaPastaPasta referenced this in commit c2fcd508a3 on May 23, 2019

PastaPastaPasta referenced this in commit 104e6fabbf on May 23, 2019

PastaPastaPasta referenced this in commit 95d40cb6b2 on May 28, 2019

PastaPastaPasta referenced this in commit e27902374d on May 28, 2019

PastaPastaPasta referenced this in commit 22d00a585a on May 28, 2019

PastaPastaPasta referenced this in commit c3994e04b8 on Jun 7, 2019

PastaPastaPasta referenced this in commit 07b4c08bd7 on Jun 8, 2019

PastaPastaPasta referenced this in commit 4c13f436af on Jun 10, 2019

barrystyle referenced this in commit 1f6f13c706 on Jan 22, 2020

random-zebra referenced this in commit 8ac733392c on Jan 25, 2021

MarcoFalke locked this on Sep 8, 2021

Wallet database handling abstractions/simplifications #9951