Scripted diff to rename some wallet classes. Motivated by discussion in #11687 (review)
| Current | New |
|---|---|
| CDBEnv | BerkeleyEnvironment |
| CDB | BerkeleyBatch |
| CWalletDBWrapper | WalletDatabase |
| CWalletDB | WalletBatch |
Berkeley* classes are intended to contain BDB specific code, while Wallet* classes are intended to be more backend-agnostic.
Also renamed associated variables:
| Current | New |
|---|---|
| dbw | database |
| pwalletdb | batch |
| pwalletdbEncryption | encrypted_batch |
Concept ACK. The current naming is very confusing. There’s even a comment in walletdb.h to say so:
0/**
1 * Overview of wallet database classes:
2 *
3 * - CDBEnv is an environment in which the database exists (has no analog in dbwrapper.h)
4 * - CWalletDBWrapper represents a wallet database (similar to CDBWrapper in dbwrapper.h)
5 * - CDB is a low-level database transaction (similar to CDBBatch in dbwrapper.h)
6 * - CWalletDB is a modifier object for the wallet, and encapsulates a database
7 * transaction as well as methods to act on the database (no analog in
8 * dbwrapper.h)
9 *
10 * The latter two are named confusingly, in contrast to what the names CDB
11 * and CWalletDB suggest they are transient transaction objects and don't
12 * represent the database itself.
13 */
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:
wallet.h?)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: 51eaec47ccb61de2d791c86b785ee4cf111c3b05. I didn’t move it into wallet.h for now because it sounds from #11687 (review) 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 003b6190e5a3d870a5eca3195e2381ee4b93de4d -> 51eaec47ccb61de2d791c86b785ee4cf111c3b05 (pr/wren.1 -> pr/wren.2, compare)
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.hwallet/walletdb.h -> wallet/schema.hI’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:
0/**
1 * Overview of wallet database classes:
2 *
3 * - WalletBatch is an abstract modifier object for the wallet database, and encapsulates a database
4 * batch update as well as methods to act on the database. It should be agnostic to the database implementation.
5 *
6 * The following classes are implementation specific:
7 * - BerkeleyEnvironment is an environment in which the database exists.
8 * - BerkeleyDatabase represents a wallet database.
9 * - BerkeleyBatch is a low-level database batch update.
10 */
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 51eaec47ccb61de2d791c86b785ee4cf111c3b05 -> 4299018f8f1f2b952c93f79b82073bcaa96807f6 (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.
s/CWallet/Wallet at the same time.
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.
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.:
0typedef BerkeleyEnvironment WalletDBEnvironment;
1typedef BerkeleyDatabase WalletDatabase;
2typedef 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.
@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
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.
Needs rebase to fix travis (sorry)
Rebased e0ad11506a62c7ad28411ba67abad68131058bf5 -> 333ede1b54f00b063a311cf8bf7d70c711fa234f (pr/wren.10 -> pr/wren.11) for #12607.
Opening comment could be updated to include WalletDatabase/BerkeleyDatabase distinction as well as the variable renames:
| Current | New |
|---|---|
| dbw | database |
| m_dbw | m_database |
| pwalletdb | batch |
| pwalletdbIn | batch_in |
| walletdb | batch |
Should pwalletdbEncryption be renamed as well?
Opening comment could be updated to include WalletDatabase/BerkeleyDatabase distinction as well as the variable renames:
Thanks, updated to note the distinction and variable renames.
Should pwalletdbEncryption be renamed as well?
Good catch. Added ren pwalletdbEncryption encrypted_batch line to the script.
Updated 333ede1b54f00b063a311cf8bf7d70c711fa234f -> e2ce4c00c141275fbbf733dc0abef1bb4d986065 (pr/wren.11 -> pr/wren.12) with just the change to the scripted-diff.
Tested ACK e2ce4c00c141275fbbf733dc0abef1bb4d986065
FYI in your scripted diff there’s ren wallet/batch.h wallet/walletdb.h which doesn’t do anything because it’s in the wrong order.
Rebased e2ce4c00c141275fbbf733dc0abef1bb4d986065 -> b21912d455f225740fc5d4356cb4b31379c077d2 (pr/wren.12 -> pr/wren.13) because of merge conflict with #10637 in wallet_tests.cpp.
FYI in your scripted diff there’s ren wallet/batch.h wallet/walletdb.h which doesn’t do anything because it’s in the wrong order.
I don’t think this is true. That line is needed to prevent the earlier ren walletdb from mangling #includes. I think it’s possible you were seeing an error from the ren dbw_in line, which is now removed because it is no longer applicable after #11687.
That line is needed to prevent the earlier ren walletdb from mangling #includes.
Ah I see, I didn’t think that through.
Tested ACK b21912d455f225740fc5d4356cb4b31379c077d2
243@@ -244,8 +244,8 @@ class CWalletDB
244 //! Write wallet version
245 bool WriteVersion(int nVersion);
246 private:
247- CDB batch;
248- CWalletDBWrapper& m_dbw;
249+ BerkeleyBatch batch;
m_batch in this PR, although that may be out of scope.
You could rename this to m_batch in this PR, although that may be out of scope.
Done in c4719633496d6be338d3c2ee3d64be613200ae82
batch -> m_batch renaming is the only change.
Needs rebase
Rebased c4719633496d6be338d3c2ee3d64be613200ae82 -> 6db9ee94fa91d3ebc864f63567314a2cce437bdd (pr/wren.15 -> pr/wren.16) due to conflicts with #11536 and #12694. Rebased 6db9ee94fa91d3ebc864f63567314a2cce437bdd -> 4dff15cd3e4cdda37b614b57278cbf4b06b26e2b (pr/wren.16 -> pr/wren.17) due to conflict with #10742.
-BEGIN VERIFY SCRIPT-
sed -i 's/\<CWalletDBWrapper\>/BerkeleyDatabase/g' src/wallet/db.h src/wallet/db.cpp
sed -i '/statuses/i/** Backend-agnostic database type. */\nusing WalletDatabase = BerkeleyDatabase\;\n' src/wallet/walletdb.h
ren() { git grep -l "\<$1\>" 'src/*.cpp' 'src/*.h' ':(exclude)*dbwrapper*' test | xargs sed -i "s:\<$1\>:$2:g"; }
ren CDBEnv BerkeleyEnvironment
ren CDB BerkeleyBatch
ren CWalletDBWrapper WalletDatabase
ren CWalletDB WalletBatch
ren dbw database
ren m_dbw m_database
ren walletdb batch
ren pwalletdb batch
ren pwalletdbIn batch_in
ren wallet/batch.h wallet/walletdb.h
ren pwalletdbEncryption encrypted_batch
-END VERIFY SCRIPT-
Text from https://github.com/bitcoin/bitcoin/pull/11851#issuecomment-350320608
by John Newbery <john@johnnewbery.com>.
I should have caught this sooner, this comment https://github.com/ryanofsky/bitcoin/blob/70caf7f7538efe1f859ce849967b7b40bbb8ae6f/src/wallet/walletdb.h#L137 (walletdb.h line 137) should be updated to reflect the renaming that happpened.
Thanks, updated coment in 3b205a89eaff2ae2b87557e6406e550e6656c4ef.
Rebased 70caf7f7538efe1f859ce849967b7b40bbb8ae6f -> 59cf1f2377f8f6315200a9fc32bbc68e2bc7184c (pr/wren.18 -> pr/wren.19) due to conflict with #12785 Added 1 commits 59cf1f2377f8f6315200a9fc32bbc68e2bc7184c -> 3b205a89eaff2ae2b87557e6406e550e6656c4ef (pr/wren.19 -> pr/wren.20, compare) Squashed 3b205a89eaff2ae2b87557e6406e550e6656c4ef -> 9b0f0c5513f74825b06a728ab9bb2fd0e4224c9f (pr/wren.20 -> pr/wren.21)
Tested ACK 9b0f0c5513f74825b06a728ab9bb2fd0e4224c9f.
This is a conflict-heavy PR. It’d be great to get this merged soon to avoid repeated rebases.