Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wallet database handling abstractions/simplifications #9951

Merged
merged 6 commits into from Apr 24, 2017

Conversation

Projects
None yet
6 participants
@laanwj
Copy link
Member

laanwj commented Mar 8, 2017

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 laanwj added the Wallet label Mar 8, 2017

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Mar 9, 2017

Great!
Concept ACK.
Needs rebase.

@laanwj laanwj force-pushed the laanwj:2017_03_wallet_dbwrapper branch Mar 9, 2017

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Mar 9, 2017

Yep needed trivial rebase for #9643.

src/wallet/walletdb.h Outdated
{
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 comment has been minimized.

@laanwj

laanwj Mar 9, 2017

Author Member

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 laanwj force-pushed the laanwj:2017_03_wallet_dbwrapper branch 2 times, most recently Mar 9, 2017

@benma
Copy link
Contributor

benma left a comment

Some comments on the first commit.

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

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

This comment has been minimized.

@benma

benma Mar 9, 2017

Contributor

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

This comment has been minimized.

@laanwj

laanwj Mar 9, 2017

Author Member

Agreed.

src/wallet/db.cpp Outdated
{
if (!dbw.env)
return true;
std::string strFile = dbw.strWalletFile;

This comment has been minimized.

@benma

benma Mar 9, 2017

Contributor

same

src/wallet/db.cpp Outdated
if (!dbw) {
return true;
}
std::string strFile = dbw->strWalletFile;

This comment has been minimized.

@benma

benma Mar 9, 2017

Contributor

same

src/wallet/db.cpp Outdated
{
bool ret = false;
if (!dbw) {

This comment has been minimized.

@benma

benma Mar 9, 2017

Contributor

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

src/wallet/db.cpp Outdated

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

This comment has been minimized.

@benma

benma Mar 9, 2017

Contributor

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

This comment has been minimized.

@laanwj

laanwj Mar 9, 2017

Author Member

Yes the brace should be after

src/wallet/db.h Outdated
std::string GetName() { return strWalletFile; }

CDBEnv *env;
std::string strWalletFile;

This comment has been minimized.

@benma

benma Mar 9, 2017

Contributor

const std::string

src/wallet/db.h Outdated

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

This comment has been minimized.

@benma

benma Mar 9, 2017

Contributor

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.

This comment has been minimized.

@laanwj

laanwj Mar 9, 2017

Author Member

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.

This comment has been minimized.

@laanwj

laanwj Mar 9, 2017

Author Member

Could make this clearer using "friend" I guess.

src/wallet/wallet.h Outdated

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

This comment has been minimized.

@benma

benma Mar 9, 2017

Contributor

GetName() const

This comment has been minimized.

@laanwj

laanwj Mar 9, 2017

Author Member

Yes good catch.

src/wallet/wallet.h Outdated
{
if (dbw) {
return dbw->GetName();
} else {

This comment has been minimized.

@benma

benma Mar 9, 2017

Contributor

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

This comment has been minimized.

@laanwj

laanwj Mar 9, 2017

Author Member

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 laanwj force-pushed the laanwj:2017_03_wallet_dbwrapper branch Mar 9, 2017

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Mar 9, 2017

Fixed @benma's nits.

@laanwj laanwj force-pushed the laanwj:2017_03_wallet_dbwrapper branch to e3e51fd Mar 9, 2017

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

This comment has been minimized.

@benma

benma Mar 9, 2017

Contributor

const std::string strFile; ?

@benma

This comment has been minimized.

Copy link
Contributor

benma commented Mar 9, 2017

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

This comment has been minimized.

Copy link
Member Author

laanwj commented Mar 9, 2017

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

This comment has been minimized.

Copy link
Member Author

laanwj commented Mar 10, 2017

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

@laanwj laanwj referenced this pull request Mar 10, 2017

Merged

Basic multiwallet support #8694

@TheBlueMatt
Copy link
Contributor

TheBlueMatt left a comment

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

utACK e3e51fd

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

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Mar 10, 2017

Contributor

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

This comment has been minimized.

@laanwj

laanwj Mar 12, 2017

Author Member

Good idea

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Mar 12, 2017

Concept ACK

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Mar 27, 2017

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

Though needs a "trivial" rebase.

@JeremyRubin
Copy link
Contributor

JeremyRubin left a comment

Couple nits, overall utack.

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

This comment has been minimized.

@JeremyRubin

JeremyRubin Apr 5, 2017

Contributor

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.

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

This comment has been minimized.

@JeremyRubin

JeremyRubin Apr 5, 2017

Contributor

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.

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

This comment has been minimized.

@JeremyRubin

JeremyRubin Apr 5, 2017

Contributor

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?

This comment has been minimized.

@laanwj

laanwj Apr 20, 2017

Author Member

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.

This comment has been minimized.

@laanwj

laanwj Apr 21, 2017

Author Member

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

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

This comment has been minimized.

@JeremyRubin

JeremyRubin Apr 5, 2017

Contributor

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

This comment has been minimized.

@laanwj

laanwj Apr 20, 2017

Author Member

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

@@ -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);

This comment has been minimized.

@JeremyRubin

JeremyRubin Apr 5, 2017

Contributor

missing std::

This comment has been minimized.

@laanwj

laanwj Apr 20, 2017

Author Member

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

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Apr 19, 2017

Needs rebase (Imo this is ready for merge).

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Apr 19, 2017

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

laanwj added some commits Mar 8, 2017

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.
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.
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.
wallet: Make IsDummy private in CWalletDBWrapper
This is only for use in the low-level functions, and CDB is already
a friend class.

@laanwj laanwj force-pushed the laanwj:2017_03_wallet_dbwrapper branch from e3e51fd to 69d2e9b Apr 20, 2017

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Apr 24, 2017

Merge #9951: Wallet database handling abstractions/simplifications
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.