wallet: Refactor BerkeleyDatabase to introduce DatabaseBatch abstract class #19325

pull achow101 wants to merge 2 commits into bitcoin:master from achow101:bdb-add-dbbatch changing 5 files +129 −90

achow101 commented at 9:02 PM on June 18, 2020: member

In order to support alternative database systems, we need to have a generic Batch class. This PR adds a DatabaseBatch abstract class which is implemented by BerkeleyBatch. DatabaseBatch is now the class that is used by WalletBatch to interact with the database. To be able to get the correct type of DatabaseBatch, BerkeleyDatabase now has a MakeBatch function which returns a newly constructed std::unique_ptr<DatabaseBatch>. For BerkeleyDatabase, that will be std::unique_ptr<BerkeleyBatch>.

The Read, Write, Erase, and Exists template functions are moved from BerkeleyBatch.

Part of #18971

Requires #19308 and #19324
achow101 cross-referenced this on Jun 18, 2020 from issue wallet: Refactor the classes in wallet/db.{cpp/h} by achow101
DrahtBot added the label Wallet on Jun 18, 2020
DrahtBot commented at 2:29 AM on June 19, 2020: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #19335 (wallet: Cleanup and separate BerkeleyDatabase and BerkeleyBatch by achow101)
- #18608 (refactor: Remove CAddressBookData::destdata by ryanofsky)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
DrahtBot cross-referenced this on Jun 19, 2020 from issue wallet: Move BerkeleyBatch static functions to BerkeleyDatabase by achow101
DrahtBot cross-referenced this on Jun 19, 2020 from issue wallet: Replace CDataStream& with CDataStream&& where appropriate by MarcoFalke
DrahtBot cross-referenced this on Jun 19, 2020 from issue wallet: BerkeleyBatch Handle cursor internally by achow101
DrahtBot cross-referenced this on Jun 19, 2020 from issue wallettool: Add dump and createfromdump commands by achow101
DrahtBot cross-referenced this on Jun 19, 2020 from issue wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101
DrahtBot cross-referenced this on Jun 19, 2020 from issue refactor: Remove CAddressBookData::destdata by ryanofsky
achow101 force-pushed on Jun 19, 2020
meshcollider added this to the "PRs" column in a project

DrahtBot cross-referenced this on Jun 19, 2020 from issue wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase by achow101
achow101 cross-referenced this on Jun 20, 2020 from issue wallet: Introduce WalletDatabase abstract class by achow101
achow101 cross-referenced this on Jun 20, 2020 from issue wallet: Cleanup and separate BerkeleyDatabase and BerkeleyBatch by achow101
achow101 force-pushed on Jun 22, 2020
achow101 force-pushed on Jun 23, 2020
ryanofsky approved
ryanofsky commented at 5:50 PM on June 23, 2020: contributor
Code review ACK de110593c212a4198285e3f4966a7e2c14185867 just last two commits:
- 6faecdf7ca995e43dd35aee8f20d5b53202b31fb walletdb: Refactor DatabaseBatch abstract class from BerkeleyBatch (5/6)
- de110593c212a4198285e3f4966a7e2c14185867 walletdb: Add MakeBatch function to BerkeleyDatabase and use it (6/6)
DrahtBot cross-referenced this on Jun 23, 2020 from issue Refactor: clean up PeriodicFlush() by jnewbery
in src/wallet/bdb.h:167 in de110593c2 outdated
```
 162 | @@ -161,6 +163,9 @@ class BerkeleyDatabase
 163 |      /** Database pointer. This is initialized lazily and reset during flushes, so it can be null. */
 164 |      std::unique_ptr<Db> m_db;
 165 |  
 166 | +    /** Make a BerkeleyBatch connected to this database */
 167 | +    std::unique_ptr<BerkeleyBatch> MakeBatch(const char* mode, bool flush_on_close);
```
promag commented at 11:02 PM on June 24, 2020:

de110593c212a4198285e3f4966a7e2c14185867

I was expecting std::unique_ptr<DatabaseBatch> here - I suppose a future refactor will make BerkeleyDatabase extends WalletDatabase and this method will be virtual?

achow101 commented at 11:24 PM on June 24, 2020:

Yes

Sjors commented at 6:38 PM on July 6, 2020:

~Probably wouldn't hurt to do that in this PR.~ (neh, doing it while adding WalletDatabase in #19334 makes sense)
promag commented at 11:12 PM on June 24, 2020: member

Code review ACK de110593c212a4198285e3f4966a7e2c14185867.

I've also reviewed base branches, those could be closed and just merge this one.
DrahtBot added the label Needs rebase on Jul 1, 2020
achow101 force-pushed on Jul 1, 2020
achow101 force-pushed on Jul 1, 2020
DrahtBot removed the label Needs rebase on Jul 1, 2020
DrahtBot added the label Needs rebase on Jul 5, 2020
achow101 force-pushed on Jul 6, 2020
Sjors commented at 6:36 PM on July 6, 2020: member

utACK aec646bb7f7d1df32675450683f97112c7881822
DrahtBot removed the label Needs rebase on Jul 6, 2020
MarcoFalke added the label Refactoring on Jul 8, 2020
DrahtBot added the label Needs rebase on Jul 8, 2020
walletdb: Refactor DatabaseBatch abstract class from BerkeleyBatch eac9200814

walletdb: Add MakeBatch function to BerkeleyDatabase and use it

Instead of having WalletBatch construct the BerkeleyBatch, have
BerkeleyDatabase do it and return a std::unique_ptr<BerkeleyBatch>

b82f0ca4d5

achow101 force-pushed on Jul 9, 2020
DrahtBot removed the label Needs rebase on Jul 9, 2020
Sjors commented at 5:00 PM on July 9, 2020: member

re-utACK b82f0ca4d5465b36debb6c57f335bdccf4899c49

Rebased after #19320; fortunately the (modified) Read, Write, Erase, Exists function bodies are just moved in the PR: git show --color-moved=dimmed-zebra -w HEAD~1.

PR description still refers to Part of [#18971](/bitcoin-bitcoin/18971/)
laanwj added this to the "Blockers" column in a project

meshcollider commented at 11:34 AM on July 11, 2020: contributor

LGTM, utACK b82f0ca4d5465b36debb6c57f335bdccf4899c49

MarcoFalke commented at 2:12 PM on July 14, 2020: member

ACK b82f0ca4d5465b36debb6c57f335bdccf4899c49 🌘

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 86e644d67a78b1c1183ee7ddb877245bbc0cc267cb631437d4367e93677e2879 -

</details>

MarcoFalke removed this from the "Blockers" column in a project

MarcoFalke merged this on Jul 14, 2020
MarcoFalke closed this on Jul 14, 2020
sidhujag referenced this in commit fa48d2e308 on Jul 14, 2020
hebasto cross-referenced this on Jul 16, 2020 from issue ci: Add tsan suppression for race in DatabaseBatch by hebasto
MarcoFalke referenced this in commit fd59670642 on Jul 17, 2020
meshcollider moved this from the "PRs" to the "Done" column in a project

meshcollider referenced this in commit 9d4b3d86b6 on Jul 23, 2020
sidhujag referenced this in commit 855dce6786 on Jul 24, 2020
Fabcien referenced this in commit 63953c6d53 on Sep 1, 2021
Fabcien referenced this in commit cbd256fb35 on Sep 1, 2021
bitcoin locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#18971 wallet: Refactor the classes in wallet/db.{cpp/h}#19308 wallet: BerkeleyBatch Handle cursor internally #19324 wallet: Move BerkeleyBatch static functions to BerkeleyDatabase #19334 wallet: Introduce WalletDatabase abstract class #19335 wallet: Cleanup and separate BerkeleyDatabase and BerkeleyBatch #19538 ci: Add tsan suppression for race in DatabaseBatch

wallet: Refactor BerkeleyDatabase to introduce DatabaseBatch abstract class #19325

Conflicts