wallet: Introduce WalletDatabase abstract class

achow101 commented at 2:21 am on June 20, 2020: member

A WalletDatabase abstract class is created from BerkeleyDatabase and is implemented by BerkeleyDatabase. First, to get to the point that this is possible, 4 functions need to be added to BerkeleyDatabase: AddRef, RemoveRef, Open, and Close.

First the increment and decrement of mapFileUseCount is refactored into separate functions AddRef and RemoveRef.

Open is introduced as a dummy function. This will raise an exception so that it always fails.

Close is refactored from Flush. The shutdown argument in Flush is removed and instead Flush(true) is now the Close function.

Split from #18971

Requires #19325

fanquake added the label Wallet on Jun 20, 2020

DrahtBot commented at 3:34 am on June 20, 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)
#19101 (refactor: remove ::vpwallets and related global variables by ryanofsky)
#18904 (Don’t call lsn_reset in periodic flush by bvbfan)

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 22, 2020

achow101 force-pushed on Jun 23, 2020

in src/wallet/bdb.cpp:331 in da7ac55e65 outdated

323@@ -324,6 +324,14 @@ void BerkeleyEnvironment::CheckpointLSN(const std::string& strFile)
324     dbenv->lsn_reset(strFile.c_str(), 0);
325 }
326 
327+BerkeleyDatabase::~BerkeleyDatabase()
328+{
329+    if (env) {
330+        size_t erased = env->m_databases.erase(strFile);

ryanofsky commented at 6:05 pm on June 23, 2020:

In commit “walletdb: Move BerkeleyDatabase::Flush(true) to Close()” (da7ac55e65926d7c0ef9b4ad8afaf96468d9574b)

Note for future cleanup: This code is just moving from the header, not changing, but want to note it seems pretty unsafe to access m_databases without locking cs_db here given multithreaded loadwallet & unloadwallet rpcs and fact that environments can be shared between wallets

ryanofsky commented at 6:42 pm on June 23, 2020:

In commit “walletdb: Move BerkeleyDatabase::Flush(true) to Close()” (da7ac55e65926d7c0ef9b4ad8afaf96468d9574b)

Note for future cleanup, this destructor should be calling CloseDb and asserting !m_db here #18971 (review) to make sure the database is actually closed

Sjors commented at 7:00 pm on July 6, 2020:

b46a69556d4c42245b60eeb4ff00ddc070669fd4: this destructor is now called from a place that did have a cs_db lock: https://github.com/bitcoin/bitcoin/pull/19334/commits/b46a69556d4c42245b60eeb4ff00ddc070669fd4#diff-f9e31f5caede7a3aa7106793ffa1e1eaL702

achow101 commented at 10:15 pm on July 6, 2020:

Added the lock back.

in src/wallet/bdb.cpp:333 in da7ac55e65 outdated

323@@ -324,6 +324,14 @@ void BerkeleyEnvironment::CheckpointLSN(const std::string& strFile)
324     dbenv->lsn_reset(strFile.c_str(), 0);
325 }
326 
327+BerkeleyDatabase::~BerkeleyDatabase()
328+{
329+    if (env) {
330+        size_t erased = env->m_databases.erase(strFile);
331+        assert(erased == 1);
332+        env->m_fileids.erase(strFile);

ryanofsky commented at 6:10 pm on June 23, 2020:

In commit “walletdb: Move BerkeleyDatabase::Flush(true) to Close()” (da7ac55e65926d7c0ef9b4ad8afaf96468d9574b)

Note for future cleanup: I didn’t test but it was reported that this erase fails sometimes #18971 (review). This seems harmless but is also unexpected. Good to debug and fix in the future.

in src/wallet/bdb.h:134 in da7ac55e65 outdated

131+    void Flush();
132+    /** Flush to the database file and close the database.
133+     *  Also close the environment if no other databases are open in it.
134      */
135-    void Flush(bool shutdown);
136+    void Close();

ryanofsky commented at 6:36 pm on June 23, 2020:

In commit “walletdb: Move BerkeleyDatabase::Flush(true) to Close()” (da7ac55e65926d7c0ef9b4ad8afaf96468d9574b)

Note for future cleanup: Descriptions and names of Close and Flush here are misleading.

I’d expect Database::Flush to reliably flush data to disk like Batch::Flush does. But Database::Flush may or may not flush data in the current database, and may flush data in other databases, since what it actually does is iterate over all databases in the environment and close ones are not in use. If the current database is in use it does nothing.

Database::Close is similarly misnamed. It basically does the same thing as Database::Flush so it or may not close the current database. The only thing it does differently is call log_archive after and remove environment log files.

To clean up this mess, I would ban use of the word “Flush” and:

Rename Batch::Flush to Batch::Checkpoint and rename Database::Flush to Database::CloseIfUnused as suggested #18971 (review)
Drop Database::Close and have a separate Archive method, or just archive logs automatically when the environment is closed as originally implemented in e172e78f8e4c70b3fe67cb152497f35587e2300c #18971 but then reverted #18971 (comment)

achow101 commented at 8:20 pm on June 23, 2020:

I would prefer to keep the naming and move to make the functions actually do as the names suggest. Since the goal is to have a standard interface, changing the names to match the craziness we have for BDB seems counterproductive. For example, in #19077, Flush and Close do work as you would expect.

I think many of these issues stem from the fact that we allow multiple BerkeleyDatabases for one BerkeleyEnvironment. I think if we stopped doing that, we would be able to make Flush and Close behave in the ways that we expect.

in src/wallet/bdb.cpp:697 in da7ac55e65 outdated

710-        if (shutdown) {
711-            LOCK(cs_db);
712-            g_dbenvs.erase(env->Directory().string());
713-            env = nullptr;
714-        } else {
715-            // TODO: To avoid g_dbenvs.erase erasing the environment prematurely after the

ryanofsky commented at 6:37 pm on June 23, 2020:

In commit “walletdb: Move BerkeleyDatabase::Flush(true) to Close()” (da7ac55e65926d7c0ef9b4ad8afaf96468d9574b)

Note: TODO is removed here because it was previously implemented #18971 (review)

in src/wallet/bdb.cpp:694 in da7ac55e65 outdated

707 {
708     if (!IsDummy()) {
709-        env->Flush(shutdown);
710-        if (shutdown) {
711-            LOCK(cs_db);
712-            g_dbenvs.erase(env->Directory().string());

ryanofsky commented at 6:40 pm on June 23, 2020:

In commit “walletdb: Move BerkeleyDatabase::Flush(true) to Close()” (da7ac55e65926d7c0ef9b4ad8afaf96468d9574b)

Note: Erase call was buggy here and is moved to destructor in this commit for reasons described #18971 (review)

in src/wallet/bdb.cpp:702 in da7ac55e65 outdated

715-            // TODO: To avoid g_dbenvs.erase erasing the environment prematurely after the
716-            // first database shutdown when multiple databases are open in the same
717-            // environment, should replace raw database `env` pointers with shared or weak
718-            // pointers, or else separate the database and environment shutdowns so
719-            // environments can be shut down after databases.
720-            env->m_fileids.erase(strFile);

ryanofsky commented at 6:42 pm on June 23, 2020:

In commit “walletdb: Move BerkeleyDatabase::Flush(true) to Close()” (da7ac55e65926d7c0ef9b4ad8afaf96468d9574b)

Note: m_fileids.erase is moving to ~Database destructor in this commit for reasons described #18971 (review)

ryanofsky commented at 7:27 pm on June 23, 2020: member

Code review ACK 4ea4ad991badd6886f8203a655d8a7c9ae437ab1, just last 4 commits:

da7ac55e65926d7c0ef9b4ad8afaf96468d9574b walletdb: Move BerkeleyDatabase::Flush(true) to Close() (7/10)
d34001d4d10a3d3accd80f71a005e92b5cbb65d0 walletdb: Introduce AddRef and RemoveRef functions (8/10)
bb64a32aef6e96fe9fb79b8abedc7becb50020f6 walletdb: Add BerkeleyDatabase::Open dummy function (9/10)
4ea4ad991badd6886f8203a655d8a7c9ae437ab1 walletdb: Introduce WalletDatabase abstract class (10/10)

These changes were originally implemented in #18971 and made bigger cleanups, but a lot of changes were reverted so the end result is messier but diffs are smaller, which is reasonable. Suggestions below are for future cleanups

achow101 commented at 8:21 pm on June 23, 2020: member

Many of the suggested cleanups are done in #19335 (as I suspect you already know)

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

in src/wallet/db.h:166 in ae35e09495 outdated

161+    /** Verifies the environment and database file */
162+    virtual bool Verify(bilingual_str& error) = 0;
163+
164+    std::string m_file_path;
165+
166+    /** Make a BerkeleyBatch connected to this database */

Sjors commented at 7:23 pm on July 6, 2020:

ae35e09495db8f5dea407053aebc05515f172469 nit : Make a DatabaseBatch

achow101 commented at 10:15 pm on July 6, 2020:

Done

DrahtBot removed the label Needs rebase on Jul 6, 2020

Sjors commented at 7:29 pm on July 6, 2020: member

utACK ae35e09495db8f5dea407053aebc05515f172469 modulo lock, and pending base PR

Glad you moved the cleanup stuff to #19335.

In walletdb.h there’s a comment: The following classes are implementation specific: BerkeleyEnvironment, BerkeleyDatabase, BerkeleyBatch. Maybe rename those?

achow101 force-pushed on Jul 6, 2020

DrahtBot added the label Needs rebase on Jul 8, 2020

achow101 force-pushed on Jul 9, 2020

DrahtBot removed the label Needs rebase on Jul 9, 2020

walletdb: Move BerkeleyDatabase::Flush(true) to Close()

Instead of having Flush optionally shutdown the database and
environment, add a Close() function that does that.

27b2766384

walletdb: Introduce AddRef and RemoveRef functions

Refactor mapFileUseCount increment and decrement to separate functions
AddRef and RemoveRef

71d28e7cdc

walletdb: Add BerkeleyDatabase::Open dummy function

Adds an Open function for the class abstraction that does nothing for
now.

2179dbcbcd

walletdb: Introduce WalletDatabase abstract class

Make WalletDatabase actually an abstract class and not just a typedef
for BerkeleyDatabase. Have BerkeleyDatabase inherit this class.

d416ae560e

achow101 force-pushed on Jul 14, 2020

laanwj added this to the "Blockers" column in a project

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

ryanofsky approved

ryanofsky commented at 8:30 pm on July 17, 2020: member

Code review ACK d416ae560e46a4846a3fd5990b7d390d2ef30ec8. Only changes since last review were rebasing after base PR #19334 merge, and adding cs_db lock in BerkeleyDatabase destructor, which should avoid races accessing env->m_databases and env->m_fileids

(Accidentally posted this review Wednesday in the wrong PR #19335#pullrequestreview-449300646)

in src/wallet/bdb.h:124 in 71d28e7cdc outdated

120@@ -121,6 +121,11 @@ class BerkeleyDatabase
121      */
122     bool Rewrite(const char* pszSkip=nullptr);
123 
124+    /** Indicate the a new database user has began using the database. */

fjahr commented at 1:17 pm on July 21, 2020:

nit for a follow-up:

0    /** Indicate that a new database user has begun using the database. */

fjahr commented at 2:07 pm on July 21, 2020: member

Code review ACK d416ae560e46a4846a3fd5990b7d390d2ef30ec8

some very minor manual testing

meshcollider commented at 3:21 am on July 23, 2020: contributor

Code review & test run ACK d416ae560e46a4846a3fd5990b7d390d2ef30ec8

meshcollider merged this on Jul 23, 2020

meshcollider closed this on Jul 23, 2020

meshcollider moved this from the "PRs" to the "Design" column in a project

meshcollider moved this from the "Design" to the "Done" column in a project

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

sidhujag referenced this in commit 855dce6786 on Jul 24, 2020

laanwj referenced this in commit 8db23349fe on Jul 29, 2020

MarcoFalke referenced this in commit 37b765b962 on Jul 30, 2020

sidhujag referenced this in commit fbe47717c0 on Jul 31, 2020

MarcoFalke referenced this in commit 143bd108ed on Dec 17, 2020

Fabcien referenced this in commit 532c362091 on Sep 2, 2021

Fabcien referenced this in commit bc20259fb8 on Sep 2, 2021

Fabcien referenced this in commit ddff120762 on Sep 2, 2021

Fabcien referenced this in commit aa3437af5c on Sep 2, 2021

DrahtBot locked this on Feb 15, 2022

wallet: Introduce WalletDatabase abstract class #19334

Conflicts