Concept ACK. The current naming is very confusing. There's even a comment in walletdb.h to say so:

/**
 * Overview of wallet database classes:
 *
 * - 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 confusingly, in contrast to what the names CDB
 * and CWalletDB suggest they are transient transaction objects and don't
 * represent the database itself.
 */

This should probably go in just before branching v0.16, since rebasing on top of this will be a pain.

Other changes you could consider:

• manually update that overview comment (and perhaps move to wallet.h?)
• rename the wallet/db and wallet/walletdb files to be more descriptive. Perhaps wallet/berkeley_db and wallet/db_checkpoint, but I'm sure we could do better than that.

I think your suggested class names are good ('checkpoint' is certainly better than 'transaction'!). I can't currently think of anything better.

manually update that overview comment (and perhaps move to wallet.h?)

Added commit to fix the overview comment: 51eaec4. I didn't move it into wallet.h for now because it sounds from #11687 (comment) like a goal is to pull database reading and writing code out of main wallet class, which would make wallet.h not the right place for it.

rename the wallet/db and wallet/walletdb files to be more descriptive. Perhaps wallet/berkeley_db and wallet/db_checkpoint, but I'm sure we could do better than that.

I'd be inclined to rename wallet/db.h to wallet/db-berkeley.h, leaving room for other implementations like wallet/db-dummy.h, wallet/db-sqlite.h, wallet/db-log.h (#5686), etc.

I'd be inclined to rename wallet/walletdb.h something like wallet/serialize.h, wallet/schema.h, wallet/kvschema.h, wallet/db.h, or wallet/db-keyvalue.h since it's mainly responsible for key value serialization and mostly not tied to the berkeleydb backend. This would make more sense with the CWalletDB -> CDB dependency removed or reversed, though.

Added 1 commits 003b619 -> 51eaec4 (pr/wren.1 -> pr/wren.2, compare)

Nice! I find the "WalletCheckpoint" name kinda confusing, it is a DB batch/transaction, so I'd kinda prefer we stick with "normal" nomenclature there. I'm really not sure that "BerkeleyCheckpoint" makes all that much sense for CDB, either. I kinda wish we'd split it out into an interface and implementation, though not sure if its required for this PR, but its already essentially a pretty clean interface towards a wallet DB backend.

I find the "WalletCheckpoint" name kinda confusing

Yeah, actually checkpoint is a bad name. I just chose it because I saw a txn_checkpoint call in CDB::Flush and assumed "checkpoint" was some kind of BDB synonym for transaction. But it's really not. It would be good to avoid the word transaction so maybe the best thing to call this is something like Batch or Update or Commit.

Can you push your latest branch onto pr/wren so it shows up here?

ACK changing Checkpoint to Batch or Commit. Please just not transaction!

ACK your suggested filename changes. My preferences would be:

• wallet/db.h -> wallet/db-berkeley.h
• wallet/walletdb.h -> wallet/schema.h

I've reviewed your commit changing the overview comment. I think it could be improved further by removing the 'analogous to x in dbwrapper.h' parts, and perhaps changing the ordering and wording, eg:

/**
 * Overview of wallet database classes:
 *
 * - WalletBatch is an abstract modifier object for the wallet database, and encapsulates a database
 *   batch update as well as methods to act on the database. It should be agnostic to the database implementation.
 *
 * The following classes are implementation specific:
 * - BerkeleyEnvironment is an environment in which the database exists.
 * - BerkeleyDatabase represents a wallet database.
 * - BerkeleyBatch is a low-level database batch update.
 */

It would be good to avoid the word transaction so maybe the best thing to call this is something like Batch or Update or Commit.

Good point. We use the term "Batch" in the context of LevelDB/CCoin stuff, so I'd vote for that.

Updated 51eaec4 -> 4299018 (pr/wren.2 -> pr/wren.3), switching from checkpoint to batch, dropping the CWallet->Wallet rename (because it isn't really related and makes the change bigger), adding some variable renames for consistency, and adding John's comment.

The only thing I didn't do yet is rename files. There might be other suggestions for names, and file renames could also happen in a separate PR.

I'm going to hold off reviewing until we're closer to a major release (since I think this only makes sense going in right before branching). Russ - let me know if you disagree and think we should be reviewing/merging sooner. If this does get merged just before a branch, then I think there's no issue with doing s/CWallet/Wallet at the same time.

Sounds good to me. It would also make things slightly easier for me if this were merged after #11687, rather than before.

I'm not sure I'd call BerkeleyBatch so low-level or recommend it be BDB-specific. Its public interface is pretty DB-agnostic - Read/Write/Verify/PeriodicFlush.

I'm not sure I'd call BerkeleyBatch so low-level or recommend it be BDB-specific. Its public interface is pretty DB-agnostic - Read/Write/Verify/PeriodicFlush.

Yeah, that code could be pulled out into WalletBatch or a new key-value store generic class. But I think John's comment is basically right. WalletBatch code should be agnostic to berkeley db details and potentially able to work with another key-value backend, while BerkeleyBatch should contain more berkeley specific stuff. This is mostly true right now, and could be made more true with some refactoring, which I can follow up with. You should talk to John though about whether you prefer this comment or the previous comment, or want to combine them somehow.

Concept ACK

Agree on the naming and using Batch.

However I agree with this as well:

I'm not sure I'd call BerkeleyBatch so low-level or recommend it be BDB-specific. Its public interface is pretty DB-agnostic - Read/Write/Verify/PeriodicFlush.

I tend to agree. I have a local patch set that ports the wallet functions to LevelDB. I don't think the interface should hardcode the database implementation name.

If you do want to do this, add a typedef so the non-database-specific code can stay without explicit Berkeley* references. e.g.:

typedef BerkeleyEnvironment WalletDBEnvironment; 
typedef BerkeleyDatabase WalletDatabase;
typedef BerkeleyBatch WalletDatabaseBatch;

(this keeps it contained; after all we don't want to have to do another batch rename every time the database changes)

@laanwj, is there a link to your leveldb patch? I'm surprised there's much code in these classes that's reusable for leveldb. BerkeleyEnvironment and BerkeleyDatabase in particular seem pretty tied to Berkeley, though BerkeleyBatch is more independent.

I'm thinking perhaps I should move the independent parts of BerkeleyBatch out into WalletBatch along with this renaming to avoid creating new confusion.

after all we don't want to have to do another batch rename every time the database changes

I can clean this up more too, but after #11687 this might not be as much of as problem as you would think. #11687 eliminates most references to the BerkeleyEnvironment, BerkeleyBatch, and BerkeleyDatabase outside of the db files.

Anyway, I think I will mark this WIP for now because there are other improvements I can make to go along with the renaming, and I think reviewer time would be better spent on #11687 anyway.

I think I found the leveldb wallet branch, it is https://github.com/laanwj/bitcoin/commits/20170310_experiment_leveldb_wallet and #9951. #5686 is also similar.

@laanwj, is there a link to your leveldb patch? I'm surprised there's much code in these classes that's reusable for leveldb.

Right - the patch replaces pretty much all the code inside the classes.
My only comment is about the exterior. So wallet.cpp etc should not be referring to Berkeley anything as class name, it doesn't have to leak through in the class naming used by the client code what database is used.

My only comment is about the exterior. So wallet.cpp etc should not be referring to Berkeley anything as class name

Ah, I see. Your concern was narrower than I thought and can be addressed by a typedef like you suggested. Will fix and have the PR ready again soon. I still might want to merge this after #11687, though since #11687 touches a lot of the same code and this being a scripted-diff is easier to rebase

Concept ACK. Was changing dbw to database in the src/test/dbwrapper_tests.cpp file intentional? That would seem to increase the scope of this PR beyond the wallet code

PSA: Everybody commenting here please stop reviewing this work in progress PR and look at #11687 instead :)

Was changing dbw to database in the src/test/dbwrapper_tests.cpp file intentional? That would seem to increase the scope of this PR beyond the wallet code

This was unintentional but seemed harmless so I just left it in. I can try to tweak the scripted-diff to prevent it.

Will fix and have the PR ready again soon.

Thanks. Sorry for being unclear at first.

Pieter was here.