Wallet database handling abstractions/simplifications #9951

laanwj · 2017-03-08T14:47:52Z

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;

jonasschnelli · 2017-03-09T08:02:52Z

Great!
Concept ACK.
Needs rebase.

laanwj · 2017-03-09T08:18:53Z

Yep needed trivial rebase for #9643.

laanwj · 2017-03-09T09:35:17Z

src/wallet/walletdb.h

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


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

benma

Some comments on the first commit.

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

benma · 2017-03-09T15:32:39Z

src/wallet/db.cpp

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


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

benma · 2017-03-09T15:32:47Z

src/wallet/db.cpp

 {
+    if (!dbw.env)
+        return true;
+    std::string strFile = dbw.strWalletFile;


benma · 2017-03-09T15:32:58Z

src/wallet/db.cpp

+    if (!dbw) {
+        return true;
+    }
+    std::string strFile = dbw->strWalletFile;


benma · 2017-03-09T15:34:08Z

src/wallet/db.cpp

 {
    bool ret = false;
+    if (!dbw) {


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

benma · 2017-03-09T15:35:16Z

src/wallet/db.cpp

+
+bool CWalletDBWrapper::Backup(const std::string& strDest)
+{
+    if (!env)


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

Yes the brace should be after

benma · 2017-03-09T15:37:01Z

src/wallet/db.h

+    std::string GetName() { return strWalletFile; }
+
+    CDBEnv *env;
+    std::string strWalletFile;


const std::string

benma · 2017-03-09T15:38:31Z

src/wallet/db.h

+
+    /** Get a name for this database, for debugging etc.
+     */
+    std::string GetName() { return strWalletFile; }


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.

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.

Could make this clearer using "friend" I guess.

benma · 2017-03-09T15:40:52Z

src/wallet/wallet.h

+
+    /** Get a name for this wallet for logging/debugging purposes.
+     */
+    std::string GetName()


GetName() const

Yes good catch.

benma · 2017-03-09T15:41:48Z

src/wallet/wallet.h

+    {
+        if (dbw) {
+            return dbw->GetName();
+        } else {


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

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.

laanwj · 2017-03-09T16:46:53Z

Fixed @benma's nits.

benma · 2017-03-09T17:20:47Z

src/wallet/db.h

+private:
+    /** BerkeleyDB specific */
+    CDBEnv *env;
+    std::string strFile;


const std::string strFile; ?

benma · 2017-03-09T18:06:13Z

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 · 2017-03-09T20:15:16Z

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 · 2017-03-10T06:46:56Z

@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.

TheBlueMatt

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

utACK e3e51fd

TheBlueMatt · 2017-03-10T20:16:45Z

src/wallet/db.h

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


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

sipa · 2017-03-12T06:35:47Z

Concept ACK

jonasschnelli · 2017-03-27T13:39:40Z

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

Though needs a "trivial" rebase.

JeremyRubin

Couple nits, overall utack.

JeremyRubin · 2017-04-05T16:51:57Z

src/wallet/db.h

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


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.

JeremyRubin · 2017-04-05T18:53:23Z

src/wallet/db.cpp

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


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.

JeremyRubin · 2017-04-05T19:10:31Z

src/wallet/db.cpp

@@ -359,13 +359,12 @@ void CDBEnv::CheckpointLSN(const std::string& strFile)
 }


-CDB::CDB(const std::string& strFilename, const char* pszMode, bool fFlushOnCloseIn) : pdb(NULL), activeTxn(NULL)
+CDB::CDB(CWalletDBWrapper& dbw, const char* pszMode, bool fFlushOnCloseIn) : pdb(NULL), activeTxn(NULL)


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?

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.

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

JeremyRubin · 2017-04-05T19:14:32Z

src/wallet/db.h

+     * Only to be used at a low level, application should ideally not care
+     * about this.
+     */
+    bool IsDummy() { return env == nullptr; }


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

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

JeremyRubin · 2017-04-05T19:20:46Z

src/wallet/walletdb.cpp

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


missing std::

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

jonasschnelli · 2017-04-19T10:23:18Z

Needs rebase (Imo this is ready for merge).

laanwj · 2017-04-19T10:44:22Z

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

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.

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.

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.

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

911a480 wallet: Add comment describing the various classes in walletdb.h (Wladimir J. van der Laan) 69d2e9b wallet: Make IsDummy private in CWalletDBWrapper (Wladimir J. van der Laan) 3323281 wallet: CWalletDB CDB composition not inheritance (Wladimir J. van der Laan) be9e1a9 wallet: Reduce references to global bitdb environment (Wladimir J. van der Laan) 071c955 wallet: Get rid of fFileBacked (Wladimir J. van der Laan) 71afe3c wallet: Introduce database handle wrapper (Wladimir J. van der Laan) Tree-SHA512: e4e72953c61a2f6995d609a32f8ed8e18cab9a92bc9e193d46a1d1f06d9daa5c6da6fce2867d4e3ba4fc0439141901a3d35f246486f0fa8f59587786379dfcbd

…ions 911a480 wallet: Add comment describing the various classes in walletdb.h (Wladimir J. van der Laan) 69d2e9b wallet: Make IsDummy private in CWalletDBWrapper (Wladimir J. van der Laan) 3323281 wallet: CWalletDB CDB composition not inheritance (Wladimir J. van der Laan) be9e1a9 wallet: Reduce references to global bitdb environment (Wladimir J. van der Laan) 071c955 wallet: Get rid of fFileBacked (Wladimir J. van der Laan) 71afe3c wallet: Introduce database handle wrapper (Wladimir J. van der Laan) Tree-SHA512: e4e72953c61a2f6995d609a32f8ed8e18cab9a92bc9e193d46a1d1f06d9daa5c6da6fce2867d4e3ba4fc0439141901a3d35f246486f0fa8f59587786379dfcbd

…ions 911a480 wallet: Add comment describing the various classes in walletdb.h (Wladimir J. van der Laan) 69d2e9b wallet: Make IsDummy private in CWalletDBWrapper (Wladimir J. van der Laan) 3323281 wallet: CWalletDB CDB composition not inheritance (Wladimir J. van der Laan) be9e1a9 wallet: Reduce references to global bitdb environment (Wladimir J. van der Laan) 071c955 wallet: Get rid of fFileBacked (Wladimir J. van der Laan) 71afe3c wallet: Introduce database handle wrapper (Wladimir J. van der Laan) Tree-SHA512: e4e72953c61a2f6995d609a32f8ed8e18cab9a92bc9e193d46a1d1f06d9daa5c6da6fce2867d4e3ba4fc0439141901a3d35f246486f0fa8f59587786379dfcbd add 'batch.' Signed-off-by: Pasta <Pasta@dash.org> remove `if (fFileBacked)` Signed-off-by: Pasta <pasta@dashboost.org> remove fFileBacked and strWalletFile Signed-off-by: Pasta <Pasta@dash.org>

…ications e5fd215 [WalletDb] Decouple backups history from the main BackupWallet function. (furszy) 1ec4b84 [WalletDb] Decouple custom backup path parsing from BackupWallet function. (furszy) 78c8679 [Cleanup] walletdb: removing all of the zerocoin related not used methods. (furszy) 53ad1f6 wallet: Add comment describing the various classes in walletdb.h (Wladimir J. van der Laan) 08d2310 wallet: Make IsDummy private in CWalletDBWrapper (Wladimir J. van der Laan) 2132f42 wallet: CWalletDB CDB composition not inheritance (Wladimir J. van der Laan) f40006a wallet: Reduce references to global bitdb environment (Wladimir J. van der Laan) e420d3d wallet: Get rid of fFileBacked (Wladimir J. van der Laan) 3f3a289 wallet: Introduce database handle wrapper (Wladimir J. van der Laan) Pull request description: Back ported bitcoin#9951 + cleaned all of the zerocoin related, not used, methods inside the walletdb class. Built on top of #2082. ACKs for top commit: random-zebra: re-utACK e5fd215 Fuzzbawls: utACK e5fd215 Tree-SHA512: 0447f6454f0ddea71e5046783d4e57d7f9774cc3a87bf454975f5a9d5e03ce371e58402ec527203f0a7373372a1a5d8260771295f5ed941067220a92560d21b1

laanwj added the Wallet label Mar 8, 2017

laanwj force-pushed the 2017_03_wallet_dbwrapper branch from c78561e to 4ab3a72 Compare March 9, 2017 08:18

laanwj commented Mar 9, 2017

View reviewed changes

laanwj force-pushed the 2017_03_wallet_dbwrapper branch 2 times, most recently from 6608924 to a2bf727 Compare March 9, 2017 15:01

benma reviewed Mar 9, 2017

View reviewed changes

laanwj force-pushed the 2017_03_wallet_dbwrapper branch from a2bf727 to 2850ed3 Compare March 9, 2017 16:46

laanwj force-pushed the 2017_03_wallet_dbwrapper branch from 2850ed3 to e3e51fd Compare March 9, 2017 17:08

benma reviewed Mar 9, 2017

View reviewed changes

src/wallet/db.h

private:

/** BerkeleyDB specific */

CDBEnv *env;

std::string strFile;

Copy link

benma Mar 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const std::string strFile; ?

laanwj mentioned this pull request Mar 10, 2017

Basic multiwallet support #8694

Merged

TheBlueMatt reviewed Mar 10, 2017

View reviewed changes

JeremyRubin approved these changes Apr 5, 2017

View reviewed changes

jonasschnelli mentioned this pull request Apr 20, 2017

wallet flushing thread could use improvement #10236

Closed

laanwj added 5 commits April 20, 2017 17:15

wallet: Introduce database handle wrapper

71afe3c

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.

wallet: Get rid of fFileBacked

071c955

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.

wallet: Reduce references to global bitdb environment

be9e1a9

wallet: CWalletDB CDB composition not inheritance

3323281

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.

wallet: Make IsDummy private in CWalletDBWrapper

69d2e9b

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

laanwj force-pushed the 2017_03_wallet_dbwrapper branch from e3e51fd to 69d2e9b Compare April 20, 2017 15:56

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

911a480

laanwj merged commit 911a480 into bitcoin:master Apr 24, 2017

ryanofsky mentioned this pull request Dec 11, 2017

scripted-diff: Rename wallet database classes #11851

Merged

furszy mentioned this pull request Jan 9, 2021

[Backport] Wallet database handling abstractions/simplifications PIVX-Project/PIVX#2126

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Wallet database handling abstractions/simplifications #9951

Wallet database handling abstractions/simplifications #9951

Uh oh!

Conversation

laanwj commented Mar 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Mar 9, 2017

Uh oh!

laanwj commented Mar 9, 2017

Uh oh!

laanwj Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benma Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Mar 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benma commented Mar 9, 2017

Uh oh!

laanwj commented Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Mar 12, 2017

Uh oh!

jonasschnelli commented Mar 27, 2017

Uh oh!

JeremyRubin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Mar 8, 2017 •

edited

Loading

laanwj Mar 9, 2017 •

edited

Loading

benma Mar 9, 2017 •

edited

Loading

laanwj Mar 9, 2017 •

edited

Loading

laanwj commented Mar 9, 2017 •

edited

Loading

laanwj commented Mar 10, 2017 •

edited

Loading

laanwj Apr 20, 2017 •

edited

Loading