wallet: Refactor BerkeleyDatabase to introduce DatabaseBatch abstract class

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

DrahtBot added the label Wallet on Jun 18, 2020

DrahtBot commented at 2:29 am on June 19, 2020: member

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.

achow101 force-pushed on Jun 19, 2020

meshcollider added this to the "PRs" column in a project

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: member

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)

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

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 🌘

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK b82f0ca4d5465b36debb6c57f335bdccf4899c49 🌘
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgGFAwAtbreGSqXICH7l9/SP2t43FqVfnwT5lC+uUMgFTy4p8sh+XuFXUgAUmPV
 8cIVlpqQW7E3lWT8C5B3ZVswgLoFgU0zkXDqXwpdCwZ9PezT5VY/N3d1CIbVVALtf
 9CwFCbpyp5tyBgFMBC4FwV5vytzIEI7eWRcA385AB8REpjEIdiEGSpUECW3kfeOMW
10w23KAobzvKS2SkqQ8/eYPjr21tkIpLpGmQiID2fgev+iUQKNviqXM9b91SwPbjas
11oOdmeDCaPpx+FT84hN/r7PvbElNKuSZoNx2oK+8ueCOje8rVOOH2ZMk8mWMoYMSa
12LYSRig5Xub085+K8mS4tMhR1jxP+NVZSQ0oZ9aYiGIh9fZvGZjVKhQnENUyHx54N
13Wtj6DT+JwtO/GyrTYfpQBRCbWLedvJ4A5uZPxDClyzXrkXOp54VyhFETsflelpUY
14pzhFLfx6hz4geRt921L/B6G0c0VvHf3L0W/uNp09pQU+2YEZRR/lUDZYPoJCYZMI
15Z3gKGlpL
16=8mPD
17-----END PGP SIGNATURE-----

Timestamp of file with hash 86e644d67a78b1c1183ee7ddb877245bbc0cc267cb631437d4367e93677e2879 -

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

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

DrahtBot locked this on Feb 15, 2022

wallet: Refactor BerkeleyDatabase to introduce DatabaseBatch abstract class #19325

Conflicts